Re: [PATCH v2 2/5] PCI: iproc: Fix up corrupted PAXC root complex config registers
From: Ray Jui
Date: Tue Jun 12 2018 - 12:57:00 EST
On 6/12/2018 5:23 AM, poza@xxxxxxxxxxxxxx wrote:
On 2018-06-12 13:57, poza@xxxxxxxxxxxxxx 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@xxxxxxxxxxxx>
---
Â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@xxxxxxxxxxxxxx>
+ÂÂÂÂÂÂÂ 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;