[PATCH v2 0/5] Improve Broadcom PAXC support

[PATCH v2 0/5] Improve Broadcom PAXC support

[Ray Jui]

This patch series improves the Broadcom PAXC support by 1) adding more
quirks for specific versions of PAXC controllers; 2) adding logic to
reject internally unconfigured physical functions from the embedded
network processor acting as endpoint; 3) reducing verbose print level
in the outbound/inbound mapping code

This patch series is based off v4.17 and is available on GIHUB:
repo: https://github.com/Broadcom/arm64-linux.git
branch: sr-paxc-v2

Changes since v1:
 - consolidate 2 PAXC related patch series into 1
 - change the way how the capability list corruption is handled, per
recommendation from Bjorn. Now handle and fix up the corruption at
the config register read
 - rebase to v4.17

Ray Jui (5):
  PCI: iproc: Activate PAXC bridge quirk for more devices
  PCI: iproc: Fix up corrupted PAXC root complex config registers
  PCI: iproc: Disable MSI parsing in certain PAXC blocks
  PCI: iproc: Reject unconfigured physical functions from PAXC
  PCI: iproc: Reduce inbound/outbound mapping print level

 drivers/pci/host/pcie-iproc.c | 159 +++++++++++++++++++++++++++++++++++-------
 drivers/pci/host/pcie-iproc.h |   8 +++
 drivers/pci/quirks.c          |   3 +
 3 files changed, 144 insertions(+), 26 deletions(-)

-- 
2.1.4

[PATCH v2 1/5] PCI: iproc: Activate PAXC bridge quirk for more devices

[Ray Jui]

Activate PAXC bridge quirk for more PAXC based PCIe root complex with
the following PCIe device ID:
0xd750, 0xd802, 0xd804

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 drivers/pci/quirks.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 2990ad1..47dfea0 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2195,6 +2195,9 @@ static void quirk_paxc_bridge(struct pci_dev *pdev)
 }
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x16cd, quirk_paxc_bridge);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x16f0, quirk_paxc_bridge);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd750, quirk_paxc_bridge);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd802, quirk_paxc_bridge);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd804, quirk_paxc_bridge);
 #endif
 
 /* Originally in EDAC sources for i82875P:
-- 
2.1.4

[PATCH v2 2/5] PCI: iproc: Fix up corrupted PAXC root complex config registers

[Ray Jui]

On certain versions of Broadcom PAXC based root complexes, certain
regions of the configuration space are corrupted. As a result, it
prevents the Linux PCIe stack from traversing the linked list of the
capability registers completely and therefore the root complex is
not advertised as "PCIe capable". This prevents the correct PCIe RID
from being parsed in the kernel PCIe stack. A correct RID is required
for mapping to a stream ID from the SMMU or the device ID from the
GICv3 ITS

This patch fixes up the issue by manually populating the related
PCIe capabilities

Signed-off-by: Ray Jui <rjui@broadcom.com>
---
 drivers/pci/host/pcie-iproc.c | 65 +++++++++++++++++++++++++++++++++++++++----
 drivers/pci/host/pcie-iproc.h |  3 ++
 2 files changed, 62 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 3c76c5f..680f6b1 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -85,6 +85,8 @@
 #define IMAP_VALID_SHIFT		0
 #define IMAP_VALID			BIT(IMAP_VALID_SHIFT)
 
+#define IPROC_PCI_PM_CAP		0x48
+#define IPROC_PCI_PM_CAP_MASK		0xffff
 #define IPROC_PCI_EXP_CAP		0xac
 
 #define IPROC_PCIE_REG_INVALID		0xffff
@@ -375,6 +377,17 @@ static const u16 iproc_pcie_reg_paxc_v2[] = {
 	[IPROC_PCIE_CFG_DATA]		= 0x1fc,
 };
 
+/*
+ * List of device IDs of controllers that have corrupted capability list that
+ * require SW fixup
+ */
+static const u16 iproc_pcie_corrupt_cap_did[] = {
+	0x16cd,
+	0x16f0,
+	0xd802,
+	0xd804
+};
+
 static inline struct iproc_pcie *iproc_data(struct pci_bus *bus)
 {
 	struct iproc_pcie *pcie = bus->sysdata;
@@ -495,6 +508,49 @@ static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
 	return data;
 }
 
+static void iproc_pcie_fix_cap(struct iproc_pcie *pcie, int where, u32 *val)
+{
+	u32 i, dev_id;
+
+	switch (where & ~0x3) {
+	case PCI_VENDOR_ID:
+		dev_id = *val >> 16;
+
+		/*
+		 * Activate fixup for those controllers that have corrupted
+		 * capability list registers
+		 */
+		for (i = 0; i < ARRAY_SIZE(iproc_pcie_corrupt_cap_did); i++)
+			if (dev_id == iproc_pcie_corrupt_cap_did[i])
+				pcie->fix_paxc_cap = true;
+		break;
+
+	case IPROC_PCI_PM_CAP:
+		if (pcie->fix_paxc_cap) {
+			/* advertise PM, force next capability to PCIe */
+			*val &= ~IPROC_PCI_PM_CAP_MASK;
+			*val |= IPROC_PCI_EXP_CAP << 8 | PCI_CAP_ID_PM;
+		}
+		break;
+
+	case IPROC_PCI_EXP_CAP:
+		if (pcie->fix_paxc_cap) {
+			/* advertise root port, version 2, terminate here */
+			*val = (PCI_EXP_TYPE_ROOT_PORT << 4 | 2) << 16 |
+				PCI_CAP_ID_EXP;
+		}
+		break;
+
+	case IPROC_PCI_EXP_CAP + PCI_EXP_RTCTL:
+		/* Don't advertise CRS SV support */
+		*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
+		break;
+
+	default:
+		break;
+	}
+}
+
 static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
 				  int where, int size, u32 *val)
 {
@@ -509,13 +565,10 @@ static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
 	/* root complex access */
 	if (busno == 0) {
 		ret = pci_generic_config_read32(bus, devfn, where, size, val);
-		if (ret != PCIBIOS_SUCCESSFUL)
-			return ret;
+		if (ret == PCIBIOS_SUCCESSFUL)
+			iproc_pcie_fix_cap(pcie, where, val);
 
-		/* Don't advertise CRS SV support */
-		if ((where & ~0x3) == IPROC_PCI_EXP_CAP + PCI_EXP_RTCTL)
-			*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
-		return PCIBIOS_SUCCESSFUL;
+		return ret;
 	}
 
 	cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
index 814b600..9d5cfee 100644
--- a/drivers/pci/host/pcie-iproc.h
+++ b/drivers/pci/host/pcie-iproc.h
@@ -60,6 +60,8 @@ struct iproc_msi;
  * @ep_is_internal: indicates an internal emulated endpoint device is connected
  * @has_apb_err_disable: indicates the controller can be configured to prevent
  * unsupported request from being forwarded as an APB bus error
+ * @fix_paxc_cap: indicates the controller has corrupted capability list in its
+ * config space registers and requires SW based fixup
  *
  * @need_ob_cfg: indicates SW needs to configure the outbound mapping window
  * @ob: outbound mapping related parameters
@@ -85,6 +87,7 @@ struct iproc_pcie {
 	int (*map_irq)(const struct pci_dev *, u8, u8);
 	bool ep_is_internal;
 	bool has_apb_err_disable;
+	bool fix_paxc_cap;
 
 	bool need_ob_cfg;
 	struct iproc_pcie_ob ob;
-- 
2.1.4

[PATCH v2 3/5] PCI: iproc: Disable MSI parsing in certain PAXC blocks

[Ray Jui]

The internal MSI parsing logic in certain revisions of PAXC root
complexes does not work properly and can casue corruptions on the
writes. They need to be disabled

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
---
 drivers/pci/host/pcie-iproc.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 680f6b1..0804aa2 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -1197,10 +1197,22 @@ static int iproc_pcie_paxb_v2_msi_steer(struct iproc_pcie *pcie, u64 msi_addr)
 	return ret;
 }
 
-static void iproc_pcie_paxc_v2_msi_steer(struct iproc_pcie *pcie, u64 msi_addr)
+static void iproc_pcie_paxc_v2_msi_steer(struct iproc_pcie *pcie, u64 msi_addr,
+					 bool enable)
 {
 	u32 val;
 
+	if (!enable) {
+		/*
+		 * Disable PAXC MSI steering. All write transfers will be
+		 * treated as non-MSI transfers
+		 */
+		val = iproc_pcie_read_reg(pcie, IPROC_PCIE_MSI_EN_CFG);
+		val &= ~MSI_ENABLE_CFG;
+		iproc_pcie_write_reg(pcie, IPROC_PCIE_MSI_EN_CFG, val);
+		return;
+	}
+
 	/*
 	 * Program bits [43:13] of address of GITS_TRANSLATER register into
 	 * bits [30:0] of the MSI base address register.  In fact, in all iProc
@@ -1254,7 +1266,7 @@ static int iproc_pcie_msi_steer(struct iproc_pcie *pcie,
 			return ret;
 		break;
 	case IPROC_PCIE_PAXC_V2:
-		iproc_pcie_paxc_v2_msi_steer(pcie, msi_addr);
+		iproc_pcie_paxc_v2_msi_steer(pcie, msi_addr, true);
 		break;
 	default:
 		return -EINVAL;
@@ -1480,6 +1492,24 @@ int iproc_pcie_remove(struct iproc_pcie *pcie)
 }
 EXPORT_SYMBOL(iproc_pcie_remove);
 
+/*
+ * The MSI parsing logic in certain revisions of Broadcom PAXC based root
+ * complex does not work and needs to be disabled
+ */
+static void quirk_paxc_disable_msi_parsing(struct pci_dev *pdev)
+{
+	struct iproc_pcie *pcie = iproc_data(pdev->bus);
+
+	if (pdev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
+		iproc_pcie_paxc_v2_msi_steer(pcie, 0, false);
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x16f0,
+			quirk_paxc_disable_msi_parsing);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd802,
+			quirk_paxc_disable_msi_parsing);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd804,
+			quirk_paxc_disable_msi_parsing);
+
 MODULE_AUTHOR("Ray Jui <rjui@broadcom.com>");
 MODULE_DESCRIPTION("Broadcom iPROC PCIe common driver");
 MODULE_LICENSE("GPL v2");
-- 
2.1.4

[PATCH v2 4/5] PCI: iproc: Reject unconfigured physical functions from PAXC

[Ray Jui]

PAXC is an emulated PCIe root complex internally in various Broadcom
based SoCs. PAXC internally connects to the embedded network processor
within these SoCs, with the embedeed network processor exposed as an
endpoint device

The number of physical functions from the embedded network processor
that can be accessed depend on the firmware configuration.
Unfortunately, due to an ASIC bug, unconfigured physical functions cannot
be properly hidden from the root complex during enumerattion. As a
result, config write access to these unconfigured physical functions
during enumeration will cause a bus lock up on the embedded network
processor

Fortunately, these unconfigured physical functions contain a very
specific, staled PCIe device ID 0x168e. By making use of this device ID,
one is able to terminate the enumeration early in the vendor/device ID
config read

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
---
 drivers/pci/host/pcie-iproc.c | 26 +++++++++++++++++++++++++-
 drivers/pci/host/pcie-iproc.h |  5 +++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 0804aa2..59be1e0 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -582,6 +582,25 @@ static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
 	if (size <= 2)
 		*val = (data >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
 
+	/*
+	 * For PAXC and PAXCv2, the total number of PFs that one can enumerate
+	 * depends on the firmware configuration. Unfortunately, due to an ASIC
+	 * bug, unconfigured PFs cannot be properly hidden from the root
+	 * complex. As a result, write access to these PFs will cause bus lock
+	 * up on the embedded processor
+	 *
+	 * Since all unconfigured PFs are left with an incorrect, staled device
+	 * ID of 0x168e (PCI_DEVICE_ID_NX2_57810), we try to catch those access
+	 * early here and reject them all
+	 */
+#define DEVICE_ID_MASK     0xffff0000
+#define DEVICE_ID_SHIFT    16
+	if (pcie->rej_unconfig_pf &&
+	    (where & CFG_ADDR_REG_NUM_MASK) == PCI_VENDOR_ID)
+		if ((*val & DEVICE_ID_MASK) ==
+		    (PCI_DEVICE_ID_NX2_57810 << DEVICE_ID_SHIFT))
+			return PCIBIOS_FUNC_NOT_SUPPORTED;
+
 	return PCIBIOS_SUCCESSFUL;
 }
 
@@ -681,7 +700,7 @@ static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
 	struct iproc_pcie *pcie = iproc_data(bus);
 
 	iproc_pcie_apb_err_disable(bus, true);
-	if (pcie->type == IPROC_PCIE_PAXB_V2)
+	if (pcie->iproc_cfg_read)
 		ret = iproc_pcie_config_read(bus, devfn, where, size, val);
 	else
 		ret = pci_generic_config_read32(bus, devfn, where, size, val);
@@ -1336,6 +1355,7 @@ static int iproc_pcie_rev_init(struct iproc_pcie *pcie)
 		break;
 	case IPROC_PCIE_PAXB:
 		regs = iproc_pcie_reg_paxb;
+		pcie->iproc_cfg_read = true;
 		pcie->has_apb_err_disable = true;
 		if (pcie->need_ob_cfg) {
 			pcie->ob_map = paxb_ob_map;
@@ -1358,10 +1378,14 @@ static int iproc_pcie_rev_init(struct iproc_pcie *pcie)
 	case IPROC_PCIE_PAXC:
 		regs = iproc_pcie_reg_paxc;
 		pcie->ep_is_internal = true;
+		pcie->iproc_cfg_read = true;
+		pcie->rej_unconfig_pf = true;
 		break;
 	case IPROC_PCIE_PAXC_V2:
 		regs = iproc_pcie_reg_paxc_v2;
 		pcie->ep_is_internal = true;
+		pcie->iproc_cfg_read = true;
+		pcie->rej_unconfig_pf = true;
 		pcie->need_msi_steer = true;
 		break;
 	default:
diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
index 9d5cfee..4f03ea5 100644
--- a/drivers/pci/host/pcie-iproc.h
+++ b/drivers/pci/host/pcie-iproc.h
@@ -58,6 +58,9 @@ struct iproc_msi;
  * @phy: optional PHY device that controls the Serdes
  * @map_irq: function callback to map interrupts
  * @ep_is_internal: indicates an internal emulated endpoint device is connected
+ * @iproc_cfg_read: indicates the iProc config read function should be used
+ * @rej_unconfig_pf: indicates the root complex needs to detect and reject
+ * enumeration against unconfigured physical functions emulated in the ASIC
  * @has_apb_err_disable: indicates the controller can be configured to prevent
  * unsupported request from being forwarded as an APB bus error
  * @fix_paxc_cap: indicates the controller has corrupted capability list in its
@@ -86,6 +89,8 @@ struct iproc_pcie {
 	struct phy *phy;
 	int (*map_irq)(const struct pci_dev *, u8, u8);
 	bool ep_is_internal;
+	bool iproc_cfg_read;
+	bool rej_unconfig_pf;
 	bool has_apb_err_disable;
 	bool fix_paxc_cap;
 
-- 
2.1.4

[PATCH v2 5/5] PCI: iproc: Reduce inbound/outbound mapping print level

[Ray Jui]

Reduce inbound/outbound mapping print level from dev_info to
dev_dbg. This reduces the console logs during Linux boot process

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
---
 drivers/pci/host/pcie-iproc.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 59be1e0..3160e93 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -880,14 +880,14 @@ static inline int iproc_pcie_ob_write(struct iproc_pcie *pcie, int window_idx,
 	writel(lower_32_bits(pci_addr), pcie->base + omap_offset);
 	writel(upper_32_bits(pci_addr), pcie->base + omap_offset + 4);
 
-	dev_info(dev, "ob window [%d]: offset 0x%x axi %pap pci %pap\n",
-		 window_idx, oarr_offset, &axi_addr, &pci_addr);
-	dev_info(dev, "oarr lo 0x%x oarr hi 0x%x\n",
-		 readl(pcie->base + oarr_offset),
-		 readl(pcie->base + oarr_offset + 4));
-	dev_info(dev, "omap lo 0x%x omap hi 0x%x\n",
-		 readl(pcie->base + omap_offset),
-		 readl(pcie->base + omap_offset + 4));
+	dev_dbg(dev, "ob window [%d]: offset 0x%x axi %pap pci %pap\n",
+		window_idx, oarr_offset, &axi_addr, &pci_addr);
+	dev_dbg(dev, "oarr lo 0x%x oarr hi 0x%x\n",
+		readl(pcie->base + oarr_offset),
+		readl(pcie->base + oarr_offset + 4));
+	dev_dbg(dev, "omap lo 0x%x omap hi 0x%x\n",
+		readl(pcie->base + omap_offset),
+		readl(pcie->base + omap_offset + 4));
 
 	return 0;
 }
@@ -1054,8 +1054,8 @@ static int iproc_pcie_ib_write(struct iproc_pcie *pcie, int region_idx,
 	    iproc_pcie_reg_is_invalid(imap_offset))
 		return -EINVAL;
 
-	dev_info(dev, "ib region [%d]: offset 0x%x axi %pap pci %pap\n",
-		 region_idx, iarr_offset, &axi_addr, &pci_addr);
+	dev_dbg(dev, "ib region [%d]: offset 0x%x axi %pap pci %pap\n",
+		region_idx, iarr_offset, &axi_addr, &pci_addr);
 
 	/*
 	 * Program the IARR registers.  The upper 32-bit IARR register is
@@ -1065,9 +1065,9 @@ static int iproc_pcie_ib_write(struct iproc_pcie *pcie, int region_idx,
 	       pcie->base + iarr_offset);
 	writel(upper_32_bits(pci_addr), pcie->base + iarr_offset + 4);
 
-	dev_info(dev, "iarr lo 0x%x iarr hi 0x%x\n",
-		 readl(pcie->base + iarr_offset),
-		 readl(pcie->base + iarr_offset + 4));
+	dev_dbg(dev, "iarr lo 0x%x iarr hi 0x%x\n",
+		readl(pcie->base + iarr_offset),
+		readl(pcie->base + iarr_offset + 4));
 
 	/*
 	 * Now program the IMAP registers.  Each IARR region may have one or
@@ -1081,10 +1081,10 @@ static int iproc_pcie_ib_write(struct iproc_pcie *pcie, int region_idx,
 		writel(upper_32_bits(axi_addr),
 		       pcie->base + imap_offset + ib_map->imap_addr_offset);
 
-		dev_info(dev, "imap window [%d] lo 0x%x hi 0x%x\n",
-			 window_idx, readl(pcie->base + imap_offset),
-			 readl(pcie->base + imap_offset +
-			       ib_map->imap_addr_offset));
+		dev_dbg(dev, "imap window [%d] lo 0x%x hi 0x%x\n",
+			window_idx, readl(pcie->base + imap_offset),
+			readl(pcie->base + imap_offset +
+			      ib_map->imap_addr_offset));
 
 		imap_offset += ib_map->imap_window_offset;
 		axi_addr += size;
-- 
2.1.4

Re: [PATCH v2 2/5] PCI: iproc: Fix up corrupted PAXC root complex config registers

[poza]

On 2018-06-12 05:51, Ray Jui wrote:
> On certain versions of Broadcom PAXC based root complexes, certain
> regions of the configuration space are corrupted. As a result, it
> prevents the Linux PCIe stack from traversing the linked list of the
> capability registers completely and therefore the root complex is
> not advertised as "PCIe capable". This prevents the correct PCIe RID
> from being parsed in the kernel PCIe stack. A correct RID is required
> for mapping to a stream ID from the SMMU or the device ID from the
> GICv3 ITS
> 
> This patch fixes up the issue by manually populating the related
> PCIe capabilities
> 
> Signed-off-by: Ray Jui <rjui@broadcom.com>
> ---
>  drivers/pci/host/pcie-iproc.c | 65 
> +++++++++++++++++++++++++++++++++++++++----
>  drivers/pci/host/pcie-iproc.h |  3 ++
>  2 files changed, 62 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-iproc.c 
> b/drivers/pci/host/pcie-iproc.c
> index 3c76c5f..680f6b1 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -85,6 +85,8 @@
>  #define IMAP_VALID_SHIFT		0
>  #define IMAP_VALID			BIT(IMAP_VALID_SHIFT)
> 
> +#define IPROC_PCI_PM_CAP		0x48
> +#define IPROC_PCI_PM_CAP_MASK		0xffff
>  #define IPROC_PCI_EXP_CAP		0xac
> 
>  #define IPROC_PCIE_REG_INVALID		0xffff
> @@ -375,6 +377,17 @@ static const u16 iproc_pcie_reg_paxc_v2[] = {
>  	[IPROC_PCIE_CFG_DATA]		= 0x1fc,
>  };
> 
> +/*
> + * List of device IDs of controllers that have corrupted capability 
> list that
> + * require SW fixup
> + */
> +static const u16 iproc_pcie_corrupt_cap_did[] = {
> +	0x16cd,
> +	0x16f0,
> +	0xd802,
> +	0xd804
> +};
> +
>  static inline struct iproc_pcie *iproc_data(struct pci_bus *bus)
>  {
>  	struct iproc_pcie *pcie = bus->sysdata;
> @@ -495,6 +508,49 @@ static unsigned int iproc_pcie_cfg_retry(void
> __iomem *cfg_data_p)
>  	return data;
>  }
> 
> +static void iproc_pcie_fix_cap(struct iproc_pcie *pcie, int where, u32 
> *val)
> +{
> +	u32 i, dev_id;
> +
> +	switch (where & ~0x3) {
> +	case PCI_VENDOR_ID:
> +		dev_id = *val >> 16;
> +
> +		/*
> +		 * Activate fixup for those controllers that have corrupted
> +		 * capability list registers
> +		 */
> +		for (i = 0; i < ARRAY_SIZE(iproc_pcie_corrupt_cap_did); i++)
> +			if (dev_id == iproc_pcie_corrupt_cap_did[i])
> +				pcie->fix_paxc_cap = true;

and I think this code will try to fix up every time config space is 
read.
Does this get corrupted often, randomly ?
Can it not be solved by using one time Quirk ?
and if not Quirk, you dont want to be setting pcie->fix_paxc_cap = false 
somewhere

besides, pcie->fix_paxc_cap = true; is set if PCI_VENDOR_ID is read 
first.
and rest cases stay with the assumption that PCI_VENDOR_ID will be read 
first.
which is infact read first during enumeration
(that is the assumption code is making), but that is safe assumption to 
make I think.

> +		break;
> +
> +	case IPROC_PCI_PM_CAP:
> +		if (pcie->fix_paxc_cap) {
> +			/* advertise PM, force next capability to PCIe */
> +			*val &= ~IPROC_PCI_PM_CAP_MASK;
> +			*val |= IPROC_PCI_EXP_CAP << 8 | PCI_CAP_ID_PM;
> +		}
> +		break;
> +
> +	case IPROC_PCI_EXP_CAP:
> +		if (pcie->fix_paxc_cap) {
> +			/* advertise root port, version 2, terminate here */
> +			*val = (PCI_EXP_TYPE_ROOT_PORT << 4 | 2) << 16 |
> +				PCI_CAP_ID_EXP;
> +		}
> +		break;
> +
> +	case IPROC_PCI_EXP_CAP + PCI_EXP_RTCTL:
> +		/* Don't advertise CRS SV support */
> +		*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
> +		break;
> +
> +	default:
> +		break;
> +	}
> +}
> +
>  static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int 
> devfn,
>  				  int where, int size, u32 *val)
>  {
> @@ -509,13 +565,10 @@ static int iproc_pcie_config_read(struct pci_bus
> *bus, unsigned int devfn,
>  	/* root complex access */
>  	if (busno == 0) {
>  		ret = pci_generic_config_read32(bus, devfn, where, size, val);
> -		if (ret != PCIBIOS_SUCCESSFUL)
> -			return ret;
> +		if (ret == PCIBIOS_SUCCESSFUL)
> +			iproc_pcie_fix_cap(pcie, where, val);
> 
> -		/* Don't advertise CRS SV support */
> -		if ((where & ~0x3) == IPROC_PCI_EXP_CAP + PCI_EXP_RTCTL)
> -			*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
> -		return PCIBIOS_SUCCESSFUL;
> +		return ret;
>  	}
> 
>  	cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
> diff --git a/drivers/pci/host/pcie-iproc.h 
> b/drivers/pci/host/pcie-iproc.h
> index 814b600..9d5cfee 100644
> --- a/drivers/pci/host/pcie-iproc.h
> +++ b/drivers/pci/host/pcie-iproc.h
> @@ -60,6 +60,8 @@ struct iproc_msi;
>   * @ep_is_internal: indicates an internal emulated endpoint device is 
> connected
>   * @has_apb_err_disable: indicates the controller can be configured to 
> prevent
>   * unsupported request from being forwarded as an APB bus error
> + * @fix_paxc_cap: indicates the controller has corrupted capability 
> list in its
> + * config space registers and requires SW based fixup
>   *
>   * @need_ob_cfg: indicates SW needs to configure the outbound mapping 
> window
>   * @ob: outbound mapping related parameters
> @@ -85,6 +87,7 @@ struct iproc_pcie {
>  	int (*map_irq)(const struct pci_dev *, u8, u8);
>  	bool ep_is_internal;
>  	bool has_apb_err_disable;
> +	bool fix_paxc_cap;
> 
>  	bool need_ob_cfg;
>  	struct iproc_pcie_ob ob;

Re: [PATCH v2 3/5] PCI: iproc: Disable MSI parsing in certain PAXC blocks

[poza]

On 2018-06-12 05:51, Ray Jui wrote:
> The internal MSI parsing logic in certain revisions of PAXC root
> complexes does not work properly and can casue corruptions on the
> writes. They need to be disabled
> 
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> ---
>  drivers/pci/host/pcie-iproc.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-iproc.c 
> b/drivers/pci/host/pcie-iproc.c
> index 680f6b1..0804aa2 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -1197,10 +1197,22 @@ static int iproc_pcie_paxb_v2_msi_steer(struct
> iproc_pcie *pcie, u64 msi_addr)
>  	return ret;
>  }
> 
> -static void iproc_pcie_paxc_v2_msi_steer(struct iproc_pcie *pcie, u64 
> msi_addr)
> +static void iproc_pcie_paxc_v2_msi_steer(struct iproc_pcie *pcie, u64 
> msi_addr,
> +					 bool enable)
>  {
>  	u32 val;
> 
> +	if (!enable) {
> +		/*
> +		 * Disable PAXC MSI steering. All write transfers will be
> +		 * treated as non-MSI transfers
> +		 */
> +		val = iproc_pcie_read_reg(pcie, IPROC_PCIE_MSI_EN_CFG);
> +		val &= ~MSI_ENABLE_CFG;
> +		iproc_pcie_write_reg(pcie, IPROC_PCIE_MSI_EN_CFG, val);
> +		return;
can be dropped.
> +	}
> +
>  	/*
>  	 * Program bits [43:13] of address of GITS_TRANSLATER register into
>  	 * bits [30:0] of the MSI base address register.  In fact, in all 
> iProc
> @@ -1254,7 +1266,7 @@ static int iproc_pcie_msi_steer(struct iproc_pcie 
> *pcie,
>  			return ret;
>  		break;
>  	case IPROC_PCIE_PAXC_V2:
> -		iproc_pcie_paxc_v2_msi_steer(pcie, msi_addr);
> +		iproc_pcie_paxc_v2_msi_steer(pcie, msi_addr, true);
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -1480,6 +1492,24 @@ int iproc_pcie_remove(struct iproc_pcie *pcie)
>  }
>  EXPORT_SYMBOL(iproc_pcie_remove);
> 
> +/*
> + * The MSI parsing logic in certain revisions of Broadcom PAXC based 
> root
> + * complex does not work and needs to be disabled
> + */
> +static void quirk_paxc_disable_msi_parsing(struct pci_dev *pdev)
> +{
> +	struct iproc_pcie *pcie = iproc_data(pdev->bus);
> +
> +	if (pdev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
> +		iproc_pcie_paxc_v2_msi_steer(pcie, 0, false);
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x16f0,
> +			quirk_paxc_disable_msi_parsing);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd802,
> +			quirk_paxc_disable_msi_parsing);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd804,
> +			quirk_paxc_disable_msi_parsing);
> +
>  MODULE_AUTHOR("Ray Jui <rjui@broadcom.com>");
>  MODULE_DESCRIPTION("Broadcom iPROC PCIe common driver");
>  MODULE_LICENSE("GPL v2");

Re: [PATCH v2 4/5] PCI: iproc: Reject unconfigured physical functions from PAXC

[poza]

On 2018-06-12 05:51, Ray Jui wrote:
> PAXC is an emulated PCIe root complex internally in various Broadcom
> based SoCs. PAXC internally connects to the embedded network processor
> within these SoCs, with the embedeed network processor exposed as an
> endpoint device
> 
> The number of physical functions from the embedded network processor
> that can be accessed depend on the firmware configuration.
> Unfortunately, due to an ASIC bug, unconfigured physical functions 
> cannot
> be properly hidden from the root complex during enumerattion. As a
> result, config write access to these unconfigured physical functions
> during enumeration will cause a bus lock up on the embedded network
> processor
> 
> Fortunately, these unconfigured physical functions contain a very
> specific, staled PCIe device ID 0x168e. By making use of this device 
> ID,
> one is able to terminate the enumeration early in the vendor/device ID
> config read
> 
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> ---
>  drivers/pci/host/pcie-iproc.c | 26 +++++++++++++++++++++++++-
>  drivers/pci/host/pcie-iproc.h |  5 +++++
>  2 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pcie-iproc.c 
> b/drivers/pci/host/pcie-iproc.c
> index 0804aa2..59be1e0 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -582,6 +582,25 @@ static int iproc_pcie_config_read(struct pci_bus
> *bus, unsigned int devfn,
>  	if (size <= 2)
>  		*val = (data >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
> 
> +	/*
> +	 * For PAXC and PAXCv2, the total number of PFs that one can 
> enumerate
> +	 * depends on the firmware configuration. Unfortunately, due to an 
> ASIC
> +	 * bug, unconfigured PFs cannot be properly hidden from the root
> +	 * complex. As a result, write access to these PFs will cause bus 
> lock
> +	 * up on the embedded processor
> +	 *
> +	 * Since all unconfigured PFs are left with an incorrect, staled 
> device
> +	 * ID of 0x168e (PCI_DEVICE_ID_NX2_57810), we try to catch those 
> access
> +	 * early here and reject them all
> +	 */
> +#define DEVICE_ID_MASK     0xffff0000
> +#define DEVICE_ID_SHIFT    16
> +	if (pcie->rej_unconfig_pf &&
> +	    (where & CFG_ADDR_REG_NUM_MASK) == PCI_VENDOR_ID)
> +		if ((*val & DEVICE_ID_MASK) ==
> +		    (PCI_DEVICE_ID_NX2_57810 << DEVICE_ID_SHIFT))
> +			return PCIBIOS_FUNC_NOT_SUPPORTED;
> +
>  	return PCIBIOS_SUCCESSFUL;
>  }
> 
> @@ -681,7 +700,7 @@ static int iproc_pcie_config_read32(struct pci_bus
> *bus, unsigned int devfn,
>  	struct iproc_pcie *pcie = iproc_data(bus);
> 
>  	iproc_pcie_apb_err_disable(bus, true);
> -	if (pcie->type == IPROC_PCIE_PAXB_V2)
> +	if (pcie->iproc_cfg_read)
>  		ret = iproc_pcie_config_read(bus, devfn, where, size, val);
>  	else
>  		ret = pci_generic_config_read32(bus, devfn, where, size, val);
> @@ -1336,6 +1355,7 @@ static int iproc_pcie_rev_init(struct iproc_pcie 
> *pcie)
>  		break;
>  	case IPROC_PCIE_PAXB:
>  		regs = iproc_pcie_reg_paxb;
> +		pcie->iproc_cfg_read = true;
>  		pcie->has_apb_err_disable = true;
>  		if (pcie->need_ob_cfg) {
>  			pcie->ob_map = paxb_ob_map;
> @@ -1358,10 +1378,14 @@ static int iproc_pcie_rev_init(struct 
> iproc_pcie *pcie)
>  	case IPROC_PCIE_PAXC:
>  		regs = iproc_pcie_reg_paxc;
>  		pcie->ep_is_internal = true;
> +		pcie->iproc_cfg_read = true;
> +		pcie->rej_unconfig_pf = true;
>  		break;
>  	case IPROC_PCIE_PAXC_V2:
>  		regs = iproc_pcie_reg_paxc_v2;
>  		pcie->ep_is_internal = true;
> +		pcie->iproc_cfg_read = true;
> +		pcie->rej_unconfig_pf = true;
>  		pcie->need_msi_steer = true;
>  		break;
>  	default:
> diff --git a/drivers/pci/host/pcie-iproc.h 
> b/drivers/pci/host/pcie-iproc.h
> index 9d5cfee..4f03ea5 100644
> --- a/drivers/pci/host/pcie-iproc.h
> +++ b/drivers/pci/host/pcie-iproc.h
> @@ -58,6 +58,9 @@ struct iproc_msi;
>   * @phy: optional PHY device that controls the Serdes
>   * @map_irq: function callback to map interrupts
>   * @ep_is_internal: indicates an internal emulated endpoint device is 
> connected
> + * @iproc_cfg_read: indicates the iProc config read function should be 
> used
> + * @rej_unconfig_pf: indicates the root complex needs to detect and 
> reject
> + * enumeration against unconfigured physical functions emulated in the 
> ASIC
>   * @has_apb_err_disable: indicates the controller can be configured to 
> prevent
>   * unsupported request from being forwarded as an APB bus error
>   * @fix_paxc_cap: indicates the controller has corrupted capability 
> list in its
> @@ -86,6 +89,8 @@ struct iproc_pcie {
>  	struct phy *phy;
>  	int (*map_irq)(const struct pci_dev *, u8, u8);
>  	bool ep_is_internal;
> +	bool iproc_cfg_read;
> +	bool rej_unconfig_pf;
>  	bool has_apb_err_disable;
>  	bool fix_paxc_cap;

Reviewed-by: Oza Pawandeep <poza@codeaurora.org>

Re: [PATCH v2 1/5] PCI: iproc: Activate PAXC bridge quirk for more devices

[poza]

On 2018-06-12 05:51, Ray Jui wrote:
> Activate PAXC bridge quirk for more PAXC based PCIe root complex with
> the following PCIe device ID:
> 0xd750, 0xd802, 0xd804
> 
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> ---
>  drivers/pci/quirks.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 2990ad1..47dfea0 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -2195,6 +2195,9 @@ static void quirk_paxc_bridge(struct pci_dev 
> *pdev)
>  }
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x16cd, 
> quirk_paxc_bridge);
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x16f0, 
> quirk_paxc_bridge);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd750, 
> quirk_paxc_bridge);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd802, 
> quirk_paxc_bridge);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd804, 
> quirk_paxc_bridge);
>  #endif
> 
>  /* Originally in EDAC sources for i82875P:

Reviewed-by: Oza Pawandeep <poza@codeaurora.org>

Re: [PATCH v2 5/5] PCI: iproc: Reduce inbound/outbound mapping print level

[poza]

On 2018-06-12 05:51, Ray Jui wrote:
> Reduce inbound/outbound mapping print level from dev_info to
> dev_dbg. This reduces the console logs during Linux boot process
> 
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> ---
>  drivers/pci/host/pcie-iproc.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-iproc.c 
> b/drivers/pci/host/pcie-iproc.c
> index 59be1e0..3160e93 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -880,14 +880,14 @@ static inline int iproc_pcie_ob_write(struct
> iproc_pcie *pcie, int window_idx,
>  	writel(lower_32_bits(pci_addr), pcie->base + omap_offset);
>  	writel(upper_32_bits(pci_addr), pcie->base + omap_offset + 4);
> 
> -	dev_info(dev, "ob window [%d]: offset 0x%x axi %pap pci %pap\n",
> -		 window_idx, oarr_offset, &axi_addr, &pci_addr);
> -	dev_info(dev, "oarr lo 0x%x oarr hi 0x%x\n",
> -		 readl(pcie->base + oarr_offset),
> -		 readl(pcie->base + oarr_offset + 4));
> -	dev_info(dev, "omap lo 0x%x omap hi 0x%x\n",
> -		 readl(pcie->base + omap_offset),
> -		 readl(pcie->base + omap_offset + 4));
> +	dev_dbg(dev, "ob window [%d]: offset 0x%x axi %pap pci %pap\n",
> +		window_idx, oarr_offset, &axi_addr, &pci_addr);
> +	dev_dbg(dev, "oarr lo 0x%x oarr hi 0x%x\n",
> +		readl(pcie->base + oarr_offset),
> +		readl(pcie->base + oarr_offset + 4));
> +	dev_dbg(dev, "omap lo 0x%x omap hi 0x%x\n",
> +		readl(pcie->base + omap_offset),
> +		readl(pcie->base + omap_offset + 4));
> 
>  	return 0;
>  }
> @@ -1054,8 +1054,8 @@ static int iproc_pcie_ib_write(struct iproc_pcie
> *pcie, int region_idx,
>  	    iproc_pcie_reg_is_invalid(imap_offset))
>  		return -EINVAL;
> 
> -	dev_info(dev, "ib region [%d]: offset 0x%x axi %pap pci %pap\n",
> -		 region_idx, iarr_offset, &axi_addr, &pci_addr);
> +	dev_dbg(dev, "ib region [%d]: offset 0x%x axi %pap pci %pap\n",
> +		region_idx, iarr_offset, &axi_addr, &pci_addr);
> 
>  	/*
>  	 * Program the IARR registers.  The upper 32-bit IARR register is
> @@ -1065,9 +1065,9 @@ static int iproc_pcie_ib_write(struct iproc_pcie
> *pcie, int region_idx,
>  	       pcie->base + iarr_offset);
>  	writel(upper_32_bits(pci_addr), pcie->base + iarr_offset + 4);
> 
> -	dev_info(dev, "iarr lo 0x%x iarr hi 0x%x\n",
> -		 readl(pcie->base + iarr_offset),
> -		 readl(pcie->base + iarr_offset + 4));
> +	dev_dbg(dev, "iarr lo 0x%x iarr hi 0x%x\n",
> +		readl(pcie->base + iarr_offset),
> +		readl(pcie->base + iarr_offset + 4));
> 
>  	/*
>  	 * Now program the IMAP registers.  Each IARR region may have one or
> @@ -1081,10 +1081,10 @@ static int iproc_pcie_ib_write(struct
> iproc_pcie *pcie, int region_idx,
>  		writel(upper_32_bits(axi_addr),
>  		       pcie->base + imap_offset + ib_map->imap_addr_offset);
> 
> -		dev_info(dev, "imap window [%d] lo 0x%x hi 0x%x\n",
> -			 window_idx, readl(pcie->base + imap_offset),
> -			 readl(pcie->base + imap_offset +
> -			       ib_map->imap_addr_offset));
> +		dev_dbg(dev, "imap window [%d] lo 0x%x hi 0x%x\n",
> +			window_idx, readl(pcie->base + imap_offset),
> +			readl(pcie->base + imap_offset +
> +			      ib_map->imap_addr_offset));
> 
>  		imap_offset += ib_map->imap_window_offset;
>  		axi_addr += size;

Reviewed-by: Oza Pawandeep <poza@codeaurora.org>

Re: [PATCH v2 2/5] PCI: iproc: Fix up corrupted PAXC root complex config registers

[poza]

On 2018-06-12 13:57, poza@codeaurora.org wrote:
> On 2018-06-12 05:51, Ray Jui wrote:
>> On certain versions of Broadcom PAXC based root complexes, certain
>> regions of the configuration space are corrupted. As a result, it
>> prevents the Linux PCIe stack from traversing the linked list of the
>> capability registers completely and therefore the root complex is
>> not advertised as "PCIe capable". This prevents the correct PCIe RID
>> from being parsed in the kernel PCIe stack. A correct RID is required
>> for mapping to a stream ID from the SMMU or the device ID from the
>> GICv3 ITS
>> 
>> This patch fixes up the issue by manually populating the related
>> PCIe capabilities
>> 
>> Signed-off-by: Ray Jui <rjui@broadcom.com>
>> ---
>>  drivers/pci/host/pcie-iproc.c | 65 
>> +++++++++++++++++++++++++++++++++++++++----
>>  drivers/pci/host/pcie-iproc.h |  3 ++
>>  2 files changed, 62 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/pci/host/pcie-iproc.c 
>> b/drivers/pci/host/pcie-iproc.c
>> index 3c76c5f..680f6b1 100644
>> --- a/drivers/pci/host/pcie-iproc.c
>> +++ b/drivers/pci/host/pcie-iproc.c
>> @@ -85,6 +85,8 @@
>>  #define IMAP_VALID_SHIFT		0
>>  #define IMAP_VALID			BIT(IMAP_VALID_SHIFT)
>> 
>> +#define IPROC_PCI_PM_CAP		0x48
>> +#define IPROC_PCI_PM_CAP_MASK		0xffff
>>  #define IPROC_PCI_EXP_CAP		0xac
>> 
>>  #define IPROC_PCIE_REG_INVALID		0xffff
>> @@ -375,6 +377,17 @@ static const u16 iproc_pcie_reg_paxc_v2[] = {
>>  	[IPROC_PCIE_CFG_DATA]		= 0x1fc,
>>  };
>> 
>> +/*
>> + * List of device IDs of controllers that have corrupted capability 
>> list that
>> + * require SW fixup
>> + */
>> +static const u16 iproc_pcie_corrupt_cap_did[] = {
>> +	0x16cd,
>> +	0x16f0,
>> +	0xd802,
>> +	0xd804
>> +};
>> +
>>  static inline struct iproc_pcie *iproc_data(struct pci_bus *bus)
>>  {
>>  	struct iproc_pcie *pcie = bus->sysdata;
>> @@ -495,6 +508,49 @@ static unsigned int iproc_pcie_cfg_retry(void
>> __iomem *cfg_data_p)
>>  	return data;
>>  }
>> 
>> +static void iproc_pcie_fix_cap(struct iproc_pcie *pcie, int where, 
>> u32 *val)
>> +{
>> +	u32 i, dev_id;
>> +
>> +	switch (where & ~0x3) {
>> +	case PCI_VENDOR_ID:
>> +		dev_id = *val >> 16;
>> +
>> +		/*
>> +		 * Activate fixup for those controllers that have corrupted
>> +		 * capability list registers
>> +		 */
>> +		for (i = 0; i < ARRAY_SIZE(iproc_pcie_corrupt_cap_did); i++)
>> +			if (dev_id == iproc_pcie_corrupt_cap_did[i])
>> +				pcie->fix_paxc_cap = true;
> 
> and I think this code will try to fix up every time config space is 
> read.
> Does this get corrupted often, randomly ?
> Can it not be solved by using one time Quirk ?
> and if not Quirk, you dont want to be setting pcie->fix_paxc_cap =
> false somewhere
> 
> besides, pcie->fix_paxc_cap = true; is set if PCI_VENDOR_ID is read 
> first.
> and rest cases stay with the assumption that PCI_VENDOR_ID will be read 
> first.
> which is infact read first during enumeration
> (that is the assumption code is making), but that is safe assumption
> to make I think.
> 

ok I see that Bjorn has suggested to fix it this way instead of Quirks.
will just mark

Reviewed-by: Oza Pawandeep <poza@codeaurora.org>

>> +		break;
>> +
>> +	case IPROC_PCI_PM_CAP:
>> +		if (pcie->fix_paxc_cap) {
>> +			/* advertise PM, force next capability to PCIe */
>> +			*val &= ~IPROC_PCI_PM_CAP_MASK;
>> +			*val |= IPROC_PCI_EXP_CAP << 8 | PCI_CAP_ID_PM;
>> +		}
>> +		break;
>> +
>> +	case IPROC_PCI_EXP_CAP:
>> +		if (pcie->fix_paxc_cap) {
>> +			/* advertise root port, version 2, terminate here */
>> +			*val = (PCI_EXP_TYPE_ROOT_PORT << 4 | 2) << 16 |
>> +				PCI_CAP_ID_EXP;
>> +		}
>> +		break;
>> +
>> +	case IPROC_PCI_EXP_CAP + PCI_EXP_RTCTL:
>> +		/* Don't advertise CRS SV support */
>> +		*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
>> +		break;
>> +
>> +	default:
>> +		break;
>> +	}
>> +}
>> +
>>  static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int 
>> devfn,
>>  				  int where, int size, u32 *val)
>>  {
>> @@ -509,13 +565,10 @@ static int iproc_pcie_config_read(struct pci_bus
>> *bus, unsigned int devfn,
>>  	/* root complex access */
>>  	if (busno == 0) {
>>  		ret = pci_generic_config_read32(bus, devfn, where, size, val);
>> -		if (ret != PCIBIOS_SUCCESSFUL)
>> -			return ret;
>> +		if (ret == PCIBIOS_SUCCESSFUL)
>> +			iproc_pcie_fix_cap(pcie, where, val);
>> 
>> -		/* Don't advertise CRS SV support */
>> -		if ((where & ~0x3) == IPROC_PCI_EXP_CAP + PCI_EXP_RTCTL)
>> -			*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
>> -		return PCIBIOS_SUCCESSFUL;
>> +		return ret;
>>  	}
>> 
>>  	cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, 
>> where);
>> diff --git a/drivers/pci/host/pcie-iproc.h 
>> b/drivers/pci/host/pcie-iproc.h
>> index 814b600..9d5cfee 100644
>> --- a/drivers/pci/host/pcie-iproc.h
>> +++ b/drivers/pci/host/pcie-iproc.h
>> @@ -60,6 +60,8 @@ struct iproc_msi;
>>   * @ep_is_internal: indicates an internal emulated endpoint device is 
>> connected
>>   * @has_apb_err_disable: indicates the controller can be configured 
>> to prevent
>>   * unsupported request from being forwarded as an APB bus error
>> + * @fix_paxc_cap: indicates the controller has corrupted capability 
>> list in its
>> + * config space registers and requires SW based fixup
>>   *
>>   * @need_ob_cfg: indicates SW needs to configure the outbound mapping 
>> window
>>   * @ob: outbound mapping related parameters
>> @@ -85,6 +87,7 @@ struct iproc_pcie {
>>  	int (*map_irq)(const struct pci_dev *, u8, u8);
>>  	bool ep_is_internal;
>>  	bool has_apb_err_disable;
>> +	bool fix_paxc_cap;
>> 
>>  	bool need_ob_cfg;
>>  	struct iproc_pcie_ob ob;

Re: [PATCH v2 2/5] PCI: iproc: Fix up corrupted PAXC root complex config registers

[Ray Jui]

On 6/12/2018 5:23 AM, poza@codeaurora.org wrote:
> On 2018-06-12 13:57, poza@codeaurora.org wrote:
>> On 2018-06-12 05:51, Ray Jui wrote:
>>> On certain versions of Broadcom PAXC based root complexes, certain
>>> regions of the configuration space are corrupted. As a result, it
>>> prevents the Linux PCIe stack from traversing the linked list of the
>>> capability registers completely and therefore the root complex is
>>> not advertised as "PCIe capable". This prevents the correct PCIe RID
>>> from being parsed in the kernel PCIe stack. A correct RID is required
>>> for mapping to a stream ID from the SMMU or the device ID from the
>>> GICv3 ITS
>>>
>>> This patch fixes up the issue by manually populating the related
>>> PCIe capabilities
>>>
>>> Signed-off-by: Ray Jui <rjui@broadcom.com>
>>> ---
>>>  drivers/pci/host/pcie-iproc.c | 65 
>>> +++++++++++++++++++++++++++++++++++++++----
>>>  drivers/pci/host/pcie-iproc.h |  3 ++
>>>  2 files changed, 62 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/pci/host/pcie-iproc.c 
>>> b/drivers/pci/host/pcie-iproc.c
>>> index 3c76c5f..680f6b1 100644
>>> --- a/drivers/pci/host/pcie-iproc.c
>>> +++ b/drivers/pci/host/pcie-iproc.c
>>> @@ -85,6 +85,8 @@
>>>  #define IMAP_VALID_SHIFT        0
>>>  #define IMAP_VALID            BIT(IMAP_VALID_SHIFT)
>>>
>>> +#define IPROC_PCI_PM_CAP        0x48
>>> +#define IPROC_PCI_PM_CAP_MASK        0xffff
>>>  #define IPROC_PCI_EXP_CAP        0xac
>>>
>>>  #define IPROC_PCIE_REG_INVALID        0xffff
>>> @@ -375,6 +377,17 @@ static const u16 iproc_pcie_reg_paxc_v2[] = {
>>>      [IPROC_PCIE_CFG_DATA]        = 0x1fc,
>>>  };
>>>
>>> +/*
>>> + * List of device IDs of controllers that have corrupted capability 
>>> list that
>>> + * require SW fixup
>>> + */
>>> +static const u16 iproc_pcie_corrupt_cap_did[] = {
>>> +    0x16cd,
>>> +    0x16f0,
>>> +    0xd802,
>>> +    0xd804
>>> +};
>>> +
>>>  static inline struct iproc_pcie *iproc_data(struct pci_bus *bus)
>>>  {
>>>      struct iproc_pcie *pcie = bus->sysdata;
>>> @@ -495,6 +508,49 @@ static unsigned int iproc_pcie_cfg_retry(void
>>> __iomem *cfg_data_p)
>>>      return data;
>>>  }
>>>
>>> +static void iproc_pcie_fix_cap(struct iproc_pcie *pcie, int where, 
>>> u32 *val)
>>> +{
>>> +    u32 i, dev_id;
>>> +
>>> +    switch (where & ~0x3) {
>>> +    case PCI_VENDOR_ID:
>>> +        dev_id = *val >> 16;
>>> +
>>> +        /*
>>> +         * Activate fixup for those controllers that have corrupted
>>> +         * capability list registers
>>> +         */
>>> +        for (i = 0; i < ARRAY_SIZE(iproc_pcie_corrupt_cap_did); i++)
>>> +            if (dev_id == iproc_pcie_corrupt_cap_did[i])
>>> +                pcie->fix_paxc_cap = true;
>>
>> and I think this code will try to fix up every time config space is read.
>> Does this get corrupted often, randomly ?
>> Can it not be solved by using one time Quirk ?
>> and if not Quirk, you dont want to be setting pcie->fix_paxc_cap =
>> false somewhere
>>
>> besides, pcie->fix_paxc_cap = true; is set if PCI_VENDOR_ID is read 
>> first.
>> and rest cases stay with the assumption that PCI_VENDOR_ID will be 
>> read first.
>> which is infact read first during enumeration
>> (that is the assumption code is making), but that is safe assumption
>> to make I think.
>>
> 
> ok I see that Bjorn has suggested to fix it this way instead of Quirks.
> will just mark

Right, and there are benefits with this approach like Bjorn has 
explained. We now have consistent lspci and config dump output using 
this approach.

> 
> Reviewed-by: Oza Pawandeep <poza@codeaurora.org>
> 
>>> +        break;
>>> +
>>> +    case IPROC_PCI_PM_CAP:
>>> +        if (pcie->fix_paxc_cap) {
>>> +            /* advertise PM, force next capability to PCIe */
>>> +            *val &= ~IPROC_PCI_PM_CAP_MASK;
>>> +            *val |= IPROC_PCI_EXP_CAP << 8 | PCI_CAP_ID_PM;
>>> +        }
>>> +        break;
>>> +
>>> +    case IPROC_PCI_EXP_CAP:
>>> +        if (pcie->fix_paxc_cap) {
>>> +            /* advertise root port, version 2, terminate here */
>>> +            *val = (PCI_EXP_TYPE_ROOT_PORT << 4 | 2) << 16 |
>>> +                PCI_CAP_ID_EXP;
>>> +        }
>>> +        break;
>>> +
>>> +    case IPROC_PCI_EXP_CAP + PCI_EXP_RTCTL:
>>> +        /* Don't advertise CRS SV support */
>>> +        *val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
>>> +        break;
>>> +
>>> +    default:
>>> +        break;
>>> +    }
>>> +}
>>> +
>>>  static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int 
>>> devfn,
>>>                    int where, int size, u32 *val)
>>>  {
>>> @@ -509,13 +565,10 @@ static int iproc_pcie_config_read(struct pci_bus
>>> *bus, unsigned int devfn,
>>>      /* root complex access */
>>>      if (busno == 0) {
>>>          ret = pci_generic_config_read32(bus, devfn, where, size, val);
>>> -        if (ret != PCIBIOS_SUCCESSFUL)
>>> -            return ret;
>>> +        if (ret == PCIBIOS_SUCCESSFUL)
>>> +            iproc_pcie_fix_cap(pcie, where, val);
>>>
>>> -        /* Don't advertise CRS SV support */
>>> -        if ((where & ~0x3) == IPROC_PCI_EXP_CAP + PCI_EXP_RTCTL)
>>> -            *val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
>>> -        return PCIBIOS_SUCCESSFUL;
>>> +        return ret;
>>>      }
>>>
>>>      cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, 
>>> where);
>>> diff --git a/drivers/pci/host/pcie-iproc.h 
>>> b/drivers/pci/host/pcie-iproc.h
>>> index 814b600..9d5cfee 100644
>>> --- a/drivers/pci/host/pcie-iproc.h
>>> +++ b/drivers/pci/host/pcie-iproc.h
>>> @@ -60,6 +60,8 @@ struct iproc_msi;
>>>   * @ep_is_internal: indicates an internal emulated endpoint device 
>>> is connected
>>>   * @has_apb_err_disable: indicates the controller can be configured 
>>> to prevent
>>>   * unsupported request from being forwarded as an APB bus error
>>> + * @fix_paxc_cap: indicates the controller has corrupted capability 
>>> list in its
>>> + * config space registers and requires SW based fixup
>>>   *
>>>   * @need_ob_cfg: indicates SW needs to configure the outbound 
>>> mapping window
>>>   * @ob: outbound mapping related parameters
>>> @@ -85,6 +87,7 @@ struct iproc_pcie {
>>>      int (*map_irq)(const struct pci_dev *, u8, u8);
>>>      bool ep_is_internal;
>>>      bool has_apb_err_disable;
>>> +    bool fix_paxc_cap;
>>>
>>>      bool need_ob_cfg;
>>>      struct iproc_pcie_ob ob;

Re: [PATCH v2 3/5] PCI: iproc: Disable MSI parsing in certain PAXC blocks

[Ray Jui]

On 6/12/2018 1:29 AM, poza@codeaurora.org wrote:
> On 2018-06-12 05:51, Ray Jui wrote:
>> The internal MSI parsing logic in certain revisions of PAXC root
>> complexes does not work properly and can casue corruptions on the
>> writes. They need to be disabled
>>
>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>> ---
>>  drivers/pci/host/pcie-iproc.c | 34 ++++++++++++++++++++++++++++++++--
>>  1 file changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/host/pcie-iproc.c 
>> b/drivers/pci/host/pcie-iproc.c
>> index 680f6b1..0804aa2 100644
>> --- a/drivers/pci/host/pcie-iproc.c
>> +++ b/drivers/pci/host/pcie-iproc.c
>> @@ -1197,10 +1197,22 @@ static int iproc_pcie_paxb_v2_msi_steer(struct
>> iproc_pcie *pcie, u64 msi_addr)
>>      return ret;
>>  }
>>
>> -static void iproc_pcie_paxc_v2_msi_steer(struct iproc_pcie *pcie, u64 
>> msi_addr)
>> +static void iproc_pcie_paxc_v2_msi_steer(struct iproc_pcie *pcie, u64 
>> msi_addr,
>> +                     bool enable)
>>  {
>>      u32 val;
>>
>> +    if (!enable) {
>> +        /*
>> +         * Disable PAXC MSI steering. All write transfers will be
>> +         * treated as non-MSI transfers
>> +         */
>> +        val = iproc_pcie_read_reg(pcie, IPROC_PCIE_MSI_EN_CFG);
>> +        val &= ~MSI_ENABLE_CFG;
>> +        iproc_pcie_write_reg(pcie, IPROC_PCIE_MSI_EN_CFG, val);
>> +        return;
> can be dropped.


No it cannot be dropped. Please review the code carefully.

Re: [PATCH v2 3/5] PCI: iproc: Disable MSI parsing in certain PAXC blocks

[poza]

On 2018-06-12 22:28, Ray Jui wrote:
> On 6/12/2018 1:29 AM, poza@codeaurora.org wrote:
>> On 2018-06-12 05:51, Ray Jui wrote:
>>> The internal MSI parsing logic in certain revisions of PAXC root
>>> complexes does not work properly and can casue corruptions on the
>>> writes. They need to be disabled
>>> 
>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>>> ---
>>>  drivers/pci/host/pcie-iproc.c | 34 
>>> ++++++++++++++++++++++++++++++++--
>>>  1 file changed, 32 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/drivers/pci/host/pcie-iproc.c 
>>> b/drivers/pci/host/pcie-iproc.c
>>> index 680f6b1..0804aa2 100644
>>> --- a/drivers/pci/host/pcie-iproc.c
>>> +++ b/drivers/pci/host/pcie-iproc.c
>>> @@ -1197,10 +1197,22 @@ static int 
>>> iproc_pcie_paxb_v2_msi_steer(struct
>>> iproc_pcie *pcie, u64 msi_addr)
>>>      return ret;
>>>  }
>>> 
>>> -static void iproc_pcie_paxc_v2_msi_steer(struct iproc_pcie *pcie, 
>>> u64 msi_addr)
>>> +static void iproc_pcie_paxc_v2_msi_steer(struct iproc_pcie *pcie, 
>>> u64 msi_addr,
>>> +                     bool enable)
>>>  {
>>>      u32 val;
>>> 
>>> +    if (!enable) {
>>> +        /*
>>> +         * Disable PAXC MSI steering. All write transfers will be
>>> +         * treated as non-MSI transfers
>>> +         */
>>> +        val = iproc_pcie_read_reg(pcie, IPROC_PCIE_MSI_EN_CFG);
>>> +        val &= ~MSI_ENABLE_CFG;
>>> +        iproc_pcie_write_reg(pcie, IPROC_PCIE_MSI_EN_CFG, val);
>>> +        return;
>> can be dropped.
> 
> 
> No it cannot be dropped. Please review the code carefully.

Ahhh, my bad, it looked like a new function to me, may e I need sleep.
sorry about that.

Reviewed-by: Oza Pawandeep <poza@codeaurora.org>

Re: [PATCH v2 0/5] Improve Broadcom PAXC support

[Ray Jui]

Hi Lorenzo/Bjorn,

Could you please help to review this patch series when you have time?

I believe I have addressed major comment in v1 from Bjorn and answered 
all questions from Lorenzo.

Thanks,

Ray

On 6/11/2018 5:21 PM, Ray Jui wrote:
> This patch series improves the Broadcom PAXC support by 1) adding more
> quirks for specific versions of PAXC controllers; 2) adding logic to
> reject internally unconfigured physical functions from the embedded
> network processor acting as endpoint; 3) reducing verbose print level
> in the outbound/inbound mapping code
> 
> This patch series is based off v4.17 and is available on GIHUB:
> repo: https://github.com/Broadcom/arm64-linux.git
> branch: sr-paxc-v2
> 
> Changes since v1:
>   - consolidate 2 PAXC related patch series into 1
>   - change the way how the capability list corruption is handled, per
> recommendation from Bjorn. Now handle and fix up the corruption at
> the config register read
>   - rebase to v4.17
> 
> Ray Jui (5):
>    PCI: iproc: Activate PAXC bridge quirk for more devices
>    PCI: iproc: Fix up corrupted PAXC root complex config registers
>    PCI: iproc: Disable MSI parsing in certain PAXC blocks
>    PCI: iproc: Reject unconfigured physical functions from PAXC
>    PCI: iproc: Reduce inbound/outbound mapping print level
> 
>   drivers/pci/host/pcie-iproc.c | 159 +++++++++++++++++++++++++++++++++++-------
>   drivers/pci/host/pcie-iproc.h |   8 +++
>   drivers/pci/quirks.c          |   3 +
>   3 files changed, 144 insertions(+), 26 deletions(-)
> 

Re: [PATCH v2 0/5] Improve Broadcom PAXC support

[Lorenzo Pieralisi]

On Wed, Jun 20, 2018 at 10:26:28AM -0700, Ray Jui wrote:
> Hi Lorenzo/Bjorn,
> 
> Could you please help to review this patch series when you have time?
> 
> I believe I have addressed major comment in v1 from Bjorn and answered all
> questions from Lorenzo.

I will have a look next week, sorry for the delay.

Thanks,
Lorenzo

> Thanks,
> 
> Ray
> 
> On 6/11/2018 5:21 PM, Ray Jui wrote:
> >This patch series improves the Broadcom PAXC support by 1) adding more
> >quirks for specific versions of PAXC controllers; 2) adding logic to
> >reject internally unconfigured physical functions from the embedded
> >network processor acting as endpoint; 3) reducing verbose print level
> >in the outbound/inbound mapping code
> >
> >This patch series is based off v4.17 and is available on GIHUB:
> >repo: https://github.com/Broadcom/arm64-linux.git
> >branch: sr-paxc-v2
> >
> >Changes since v1:
> >  - consolidate 2 PAXC related patch series into 1
> >  - change the way how the capability list corruption is handled, per
> >recommendation from Bjorn. Now handle and fix up the corruption at
> >the config register read
> >  - rebase to v4.17
> >
> >Ray Jui (5):
> >   PCI: iproc: Activate PAXC bridge quirk for more devices
> >   PCI: iproc: Fix up corrupted PAXC root complex config registers
> >   PCI: iproc: Disable MSI parsing in certain PAXC blocks
> >   PCI: iproc: Reject unconfigured physical functions from PAXC
> >   PCI: iproc: Reduce inbound/outbound mapping print level
> >
> >  drivers/pci/host/pcie-iproc.c | 159 +++++++++++++++++++++++++++++++++++-------
> >  drivers/pci/host/pcie-iproc.h |   8 +++
> >  drivers/pci/quirks.c          |   3 +
> >  3 files changed, 144 insertions(+), 26 deletions(-)
> >

Re: [PATCH v2 0/5] Improve Broadcom PAXC support

[Ray Jui]

Hi Lorenzo,

On 6/21/2018 9:48 AM, Lorenzo Pieralisi wrote:
> On Wed, Jun 20, 2018 at 10:26:28AM -0700, Ray Jui wrote:
>> Hi Lorenzo/Bjorn,
>>
>> Could you please help to review this patch series when you have time?
>>
>> I believe I have addressed major comment in v1 from Bjorn and answered all
>> questions from Lorenzo.
> 
> I will have a look next week, sorry for the delay.

Thanks a lot. Much appreciated!

Ray

> 
> Thanks,
> Lorenzo
> 
>> Thanks,
>>
>> Ray
>>
>> On 6/11/2018 5:21 PM, Ray Jui wrote:
>>> This patch series improves the Broadcom PAXC support by 1) adding more
>>> quirks for specific versions of PAXC controllers; 2) adding logic to
>>> reject internally unconfigured physical functions from the embedded
>>> network processor acting as endpoint; 3) reducing verbose print level
>>> in the outbound/inbound mapping code
>>>
>>> This patch series is based off v4.17 and is available on GIHUB:
>>> repo: https://github.com/Broadcom/arm64-linux.git
>>> branch: sr-paxc-v2
>>>
>>> Changes since v1:
>>>   - consolidate 2 PAXC related patch series into 1
>>>   - change the way how the capability list corruption is handled, per
>>> recommendation from Bjorn. Now handle and fix up the corruption at
>>> the config register read
>>>   - rebase to v4.17
>>>
>>> Ray Jui (5):
>>>    PCI: iproc: Activate PAXC bridge quirk for more devices
>>>    PCI: iproc: Fix up corrupted PAXC root complex config registers
>>>    PCI: iproc: Disable MSI parsing in certain PAXC blocks
>>>    PCI: iproc: Reject unconfigured physical functions from PAXC
>>>    PCI: iproc: Reduce inbound/outbound mapping print level
>>>
>>>   drivers/pci/host/pcie-iproc.c | 159 +++++++++++++++++++++++++++++++++++-------
>>>   drivers/pci/host/pcie-iproc.h |   8 +++
>>>   drivers/pci/quirks.c          |   3 +
>>>   3 files changed, 144 insertions(+), 26 deletions(-)
>>>

Re: [PATCH v2 0/5] Improve Broadcom PAXC support

[Ray Jui]

Hi Lorenzo,

A friendly reminder: Have you had a chance to help to review the patch 
series below?

Thanks,

Ray

On 6/21/2018 11:22 AM, Ray Jui wrote:
> Hi Lorenzo,
> 
> On 6/21/2018 9:48 AM, Lorenzo Pieralisi wrote:
>> On Wed, Jun 20, 2018 at 10:26:28AM -0700, Ray Jui wrote:
>>> Hi Lorenzo/Bjorn,
>>>
>>> Could you please help to review this patch series when you have time?
>>>
>>> I believe I have addressed major comment in v1 from Bjorn and 
>>> answered all
>>> questions from Lorenzo.
>>
>> I will have a look next week, sorry for the delay.
> 
> Thanks a lot. Much appreciated!
> 
> Ray
> 
>>
>> Thanks,
>> Lorenzo
>>
>>> Thanks,
>>>
>>> Ray
>>>
>>> On 6/11/2018 5:21 PM, Ray Jui wrote:
>>>> This patch series improves the Broadcom PAXC support by 1) adding more
>>>> quirks for specific versions of PAXC controllers; 2) adding logic to
>>>> reject internally unconfigured physical functions from the embedded
>>>> network processor acting as endpoint; 3) reducing verbose print level
>>>> in the outbound/inbound mapping code
>>>>
>>>> This patch series is based off v4.17 and is available on GIHUB:
>>>> repo: https://github.com/Broadcom/arm64-linux.git
>>>> branch: sr-paxc-v2
>>>>
>>>> Changes since v1:
>>>>   - consolidate 2 PAXC related patch series into 1
>>>>   - change the way how the capability list corruption is handled, per
>>>> recommendation from Bjorn. Now handle and fix up the corruption at
>>>> the config register read
>>>>   - rebase to v4.17
>>>>
>>>> Ray Jui (5):
>>>>    PCI: iproc: Activate PAXC bridge quirk for more devices
>>>>    PCI: iproc: Fix up corrupted PAXC root complex config registers
>>>>    PCI: iproc: Disable MSI parsing in certain PAXC blocks
>>>>    PCI: iproc: Reject unconfigured physical functions from PAXC
>>>>    PCI: iproc: Reduce inbound/outbound mapping print level
>>>>
>>>>   drivers/pci/host/pcie-iproc.c | 159 
>>>> +++++++++++++++++++++++++++++++++++-------
>>>>   drivers/pci/host/pcie-iproc.h |   8 +++
>>>>   drivers/pci/quirks.c          |   3 +
>>>>   3 files changed, 144 insertions(+), 26 deletions(-)
>>>>

Re: [PATCH v2 0/5] Improve Broadcom PAXC support

[Lorenzo Pieralisi]

On Mon, Jun 11, 2018 at 05:21:02PM -0700, Ray Jui wrote:
> This patch series improves the Broadcom PAXC support by 1) adding more
> quirks for specific versions of PAXC controllers; 2) adding logic to
> reject internally unconfigured physical functions from the embedded
> network processor acting as endpoint; 3) reducing verbose print level
> in the outbound/inbound mapping code

Nit: commit log sentences must be terminated with periods, that's valid
for all patches in your series inclusive of this cover letter, I can
change them myself but pointing this out so that you will be able
to do it yourself next time.

Lorenzo

> This patch series is based off v4.17 and is available on GIHUB:
> repo: https://github.com/Broadcom/arm64-linux.git
> branch: sr-paxc-v2
> 
> Changes since v1:
>  - consolidate 2 PAXC related patch series into 1
>  - change the way how the capability list corruption is handled, per
> recommendation from Bjorn. Now handle and fix up the corruption at
> the config register read
>  - rebase to v4.17
> 
> Ray Jui (5):
>   PCI: iproc: Activate PAXC bridge quirk for more devices
>   PCI: iproc: Fix up corrupted PAXC root complex config registers
>   PCI: iproc: Disable MSI parsing in certain PAXC blocks
>   PCI: iproc: Reject unconfigured physical functions from PAXC
>   PCI: iproc: Reduce inbound/outbound mapping print level
> 
>  drivers/pci/host/pcie-iproc.c | 159 +++++++++++++++++++++++++++++++++++-------
>  drivers/pci/host/pcie-iproc.h |   8 +++
>  drivers/pci/quirks.c          |   3 +
>  3 files changed, 144 insertions(+), 26 deletions(-)
> 
> -- 
> 2.1.4
> 

Re: [PATCH v2 0/5] Improve Broadcom PAXC support

[Ray Jui]

Hi Lorenzo,

On 7/6/2018 9:20 AM, Lorenzo Pieralisi wrote:
> On Mon, Jun 11, 2018 at 05:21:02PM -0700, Ray Jui wrote:
>> This patch series improves the Broadcom PAXC support by 1) adding more
>> quirks for specific versions of PAXC controllers; 2) adding logic to
>> reject internally unconfigured physical functions from the embedded
>> network processor acting as endpoint; 3) reducing verbose print level
>> in the outbound/inbound mapping code
> 
> Nit: commit log sentences must be terminated with periods, that's valid
> for all patches in your series inclusive of this cover letter, I can
> change them myself but pointing this out so that you will be able
> to do it yourself next time.
> 
> Lorenzo
> 

Got it. I'll make sure commit message sentences are terminated with 
periods in the future.

Thanks a lot!

Ray

>> This patch series is based off v4.17 and is available on GIHUB:
>> repo: https://github.com/Broadcom/arm64-linux.git
>> branch: sr-paxc-v2
>>
>> Changes since v1:
>>   - consolidate 2 PAXC related patch series into 1
>>   - change the way how the capability list corruption is handled, per
>> recommendation from Bjorn. Now handle and fix up the corruption at
>> the config register read
>>   - rebase to v4.17
>>
>> Ray Jui (5):
>>    PCI: iproc: Activate PAXC bridge quirk for more devices
>>    PCI: iproc: Fix up corrupted PAXC root complex config registers
>>    PCI: iproc: Disable MSI parsing in certain PAXC blocks
>>    PCI: iproc: Reject unconfigured physical functions from PAXC
>>    PCI: iproc: Reduce inbound/outbound mapping print level
>>
>>   drivers/pci/host/pcie-iproc.c | 159 +++++++++++++++++++++++++++++++++++-------
>>   drivers/pci/host/pcie-iproc.h |   8 +++
>>   drivers/pci/quirks.c          |   3 +
>>   3 files changed, 144 insertions(+), 26 deletions(-)
>>
>> -- 
>> 2.1.4
>>

Re: [PATCH v2 0/5] Improve Broadcom PAXC support

[Lorenzo Pieralisi]

On Mon, Jun 11, 2018 at 05:21:02PM -0700, Ray Jui wrote:
> This patch series improves the Broadcom PAXC support by 1) adding more
> quirks for specific versions of PAXC controllers; 2) adding logic to
> reject internally unconfigured physical functions from the embedded
> network processor acting as endpoint; 3) reducing verbose print level
> in the outbound/inbound mapping code
> 
> This patch series is based off v4.17 and is available on GIHUB:
> repo: https://github.com/Broadcom/arm64-linux.git
> branch: sr-paxc-v2
> 
> Changes since v1:
>  - consolidate 2 PAXC related patch series into 1
>  - change the way how the capability list corruption is handled, per
> recommendation from Bjorn. Now handle and fix up the corruption at
> the config register read
>  - rebase to v4.17
> 
> Ray Jui (5):
>   PCI: iproc: Activate PAXC bridge quirk for more devices
>   PCI: iproc: Fix up corrupted PAXC root complex config registers
>   PCI: iproc: Disable MSI parsing in certain PAXC blocks
>   PCI: iproc: Reject unconfigured physical functions from PAXC
>   PCI: iproc: Reduce inbound/outbound mapping print level
> 
>  drivers/pci/host/pcie-iproc.c | 159 +++++++++++++++++++++++++++++++++++-------
>  drivers/pci/host/pcie-iproc.h |   8 +++
>  drivers/pci/quirks.c          |   3 +
>  3 files changed, 144 insertions(+), 26 deletions(-)

Hi Ray,

apart from patch 1, that requires Bjorn's ACK, I would take the
series (I will rewrite the logs), I would appreciate if the amount
of HW quirks would decrease since it is becoming quite unwieldy to
handle them, it is your code but please get the point across.

Lorenzo

Re: [PATCH v2 0/5] Improve Broadcom PAXC support

[Ray Jui]

Hi Lorenzo,

On 7/9/2018 10:22 AM, Lorenzo Pieralisi wrote:
> On Mon, Jun 11, 2018 at 05:21:02PM -0700, Ray Jui wrote:
>> This patch series improves the Broadcom PAXC support by 1) adding more
>> quirks for specific versions of PAXC controllers; 2) adding logic to
>> reject internally unconfigured physical functions from the embedded
>> network processor acting as endpoint; 3) reducing verbose print level
>> in the outbound/inbound mapping code
>>
>> This patch series is based off v4.17 and is available on GIHUB:
>> repo: https://github.com/Broadcom/arm64-linux.git
>> branch: sr-paxc-v2
>>
>> Changes since v1:
>>   - consolidate 2 PAXC related patch series into 1
>>   - change the way how the capability list corruption is handled, per
>> recommendation from Bjorn. Now handle and fix up the corruption at
>> the config register read
>>   - rebase to v4.17
>>
>> Ray Jui (5):
>>    PCI: iproc: Activate PAXC bridge quirk for more devices
>>    PCI: iproc: Fix up corrupted PAXC root complex config registers
>>    PCI: iproc: Disable MSI parsing in certain PAXC blocks
>>    PCI: iproc: Reject unconfigured physical functions from PAXC
>>    PCI: iproc: Reduce inbound/outbound mapping print level
>>
>>   drivers/pci/host/pcie-iproc.c | 159 +++++++++++++++++++++++++++++++++++-------
>>   drivers/pci/host/pcie-iproc.h |   8 +++
>>   drivers/pci/quirks.c          |   3 +
>>   3 files changed, 144 insertions(+), 26 deletions(-)
> 
> Hi Ray,
> 
> apart from patch 1, that requires Bjorn's ACK, I would take the
> series (I will rewrite the logs), I would appreciate if the amount
> of HW quirks would decrease since it is becoming quite unwieldy to
> handle them, it is your code but please get the point across.
> 
> Lorenzo
> 

Okay, thanks Lorenzo. I will ping Bjorn for patch 1.

And yes, the amount of HW quirks is overwhelming. We do have a process 
internally to track each quirk and a plan to address them in the next 
revision of the silicon based on priority, but that's largely managed by 
our ASIC team. Bottom line is the next revision of the ASIC should 
require much less of these quirks though.

Thanks,

Ray

Re: [PATCH v2 1/5] PCI: iproc: Activate PAXC bridge quirk for more devices

[Ray Jui]

Hi Bjorn,

Could you please help to review/ack this patch, based on the following 
comments from Lorenzo?

 > apart from patch 1, that requires Bjorn's ACK, I would take the
 > series (I will rewrite the logs)

Thanks,

Ray

On 6/12/2018 1:30 AM, poza@codeaurora.org wrote:
> On 2018-06-12 05:51, Ray Jui wrote:
>> Activate PAXC bridge quirk for more PAXC based PCIe root complex with
>> the following PCIe device ID:
>> 0xd750, 0xd802, 0xd804
>>
>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>> ---
>>  drivers/pci/quirks.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 2990ad1..47dfea0 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -2195,6 +2195,9 @@ static void quirk_paxc_bridge(struct pci_dev *pdev)
>>  }
>>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x16cd, 
>> quirk_paxc_bridge);
>>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x16f0, 
>> quirk_paxc_bridge);
>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd750, 
>> quirk_paxc_bridge);
>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd802, 
>> quirk_paxc_bridge);
>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd804, 
>> quirk_paxc_bridge);
>>  #endif
>>
>>  /* Originally in EDAC sources for i82875P:
> 
> Reviewed-by: Oza Pawandeep <poza@codeaurora.org>

Re: [PATCH v2 1/5] PCI: iproc: Activate PAXC bridge quirk for more devices

[Bjorn Helgaas]

On Mon, Jun 11, 2018 at 05:21:03PM -0700, Ray Jui wrote:
> Activate PAXC bridge quirk for more PAXC based PCIe root complex with
> the following PCIe device ID:
> 0xd750, 0xd802, 0xd804
> 
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>

Because this quirk_paxc_bridge() mechanism is already established,

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

BUT, I would push back on quirk_paxc_bridge() if it were new code.

I think it would be much better to implement this in the driver's
config accessors so lspci would show the correct things and the
generic code that deals with pcie_mpss would work unmodified.

> ---
>  drivers/pci/quirks.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 2990ad1..47dfea0 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -2195,6 +2195,9 @@ static void quirk_paxc_bridge(struct pci_dev *pdev)
>  }
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x16cd, quirk_paxc_bridge);
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x16f0, quirk_paxc_bridge);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd750, quirk_paxc_bridge);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd802, quirk_paxc_bridge);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd804, quirk_paxc_bridge);
>  #endif
>  
>  /* Originally in EDAC sources for i82875P:
> -- 
> 2.1.4
> 

Re: [PATCH v2 1/5] PCI: iproc: Activate PAXC bridge quirk for more devices

[Ray Jui]

Hi Bjorn,

On 7/11/2018 6:11 AM, Bjorn Helgaas wrote:
> On Mon, Jun 11, 2018 at 05:21:03PM -0700, Ray Jui wrote:
>> Activate PAXC bridge quirk for more PAXC based PCIe root complex with
>> the following PCIe device ID:
>> 0xd750, 0xd802, 0xd804
>>
>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> 
> Because this quirk_paxc_bridge() mechanism is already established,
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 

Thanks!

> BUT, I would push back on quirk_paxc_bridge() if it were new code.
> 
> I think it would be much better to implement this in the driver's
> config accessors so lspci would show the correct things and the
> generic code that deals with pcie_mpss would work unmodified.
>

Noted.

I agree with you. I'll find time to improve this by moving them into the 
driver's config accessors in the future, after v4.19.

>> ---
>>   drivers/pci/quirks.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 2990ad1..47dfea0 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -2195,6 +2195,9 @@ static void quirk_paxc_bridge(struct pci_dev *pdev)
>>   }
>>   DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x16cd, quirk_paxc_bridge);
>>   DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x16f0, quirk_paxc_bridge);
>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd750, quirk_paxc_bridge);
>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd802, quirk_paxc_bridge);
>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd804, quirk_paxc_bridge);
>>   #endif
>>   
>>   /* Originally in EDAC sources for i82875P:
>> -- 
>> 2.1.4
>>

Re: [PATCH v2 0/5] Improve Broadcom PAXC support

[Lorenzo Pieralisi]

On Mon, Jun 11, 2018 at 05:21:02PM -0700, Ray Jui wrote:
> This patch series improves the Broadcom PAXC support by 1) adding more
> quirks for specific versions of PAXC controllers; 2) adding logic to
> reject internally unconfigured physical functions from the embedded
> network processor acting as endpoint; 3) reducing verbose print level
> in the outbound/inbound mapping code
> 
> This patch series is based off v4.17 and is available on GIHUB:
> repo: https://github.com/Broadcom/arm64-linux.git
> branch: sr-paxc-v2
> 
> Changes since v1:
>  - consolidate 2 PAXC related patch series into 1
>  - change the way how the capability list corruption is handled, per
> recommendation from Bjorn. Now handle and fix up the corruption at
> the config register read
>  - rebase to v4.17
> 
> Ray Jui (5):
>   PCI: iproc: Activate PAXC bridge quirk for more devices
>   PCI: iproc: Fix up corrupted PAXC root complex config registers
>   PCI: iproc: Disable MSI parsing in certain PAXC blocks
>   PCI: iproc: Reject unconfigured physical functions from PAXC
>   PCI: iproc: Reduce inbound/outbound mapping print level
> 
>  drivers/pci/host/pcie-iproc.c | 159 +++++++++++++++++++++++++++++++++++-------
>  drivers/pci/host/pcie-iproc.h |   8 +++
>  drivers/pci/quirks.c          |   3 +
>  3 files changed, 144 insertions(+), 26 deletions(-)

Applied to pci/iproc with Bjorn's ACK for v4.19, thanks.

Lorenzo