Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
From: Lorenzo Pieralisi <hidden>
Date: 2016-06-20 09:42:07
On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi [off-list ref] wrote:quoted
On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:quoted
From: Tomasz Nowicki <redacted> pci_generic_ecam_ops is used by default. Since there are platforms which have non-compliant ECAM space we need to overwrite these accessors prior to PCI buses enumeration. In order to do that we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that we can use proper PCI config space accessors and bus_shift. pci_generic_ecam_ops is still used for platforms free from quirks. Signed-off-by: Tomasz Nowicki <redacted> --- arch/arm64/kernel/pci.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index 94cd43c..a891bda 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c@@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root) struct pci_config_window *cfg; struct resource cfgres; unsigned int bsz; + struct pci_ecam_ops *ops; /* Use address from _CBA if present, otherwise lookup MCFG */ if (!root->mcfg_addr)@@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root) return NULL; } - bsz = 1 << pci_generic_ecam_ops.bus_shift; + ops = pci_mcfg_get_ops(root); + bsz = 1 << ops->bus_shift; cfgres.start = root->mcfg_addr + bus_res->start * bsz; cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1; cfgres.flags = IORESOURCE_MEM; - cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, - &pci_generic_ecam_ops); + cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, ops);Arnd pointed this out already, I think that's the only pending question here. pci_ecam_create() maps ECAM space for config regions retrieved from the MCFG, which are *supposed* to be ECAM compliant. Do we think that's *always* correct/safe regardless of the kind of quirk we are currently fixing up ? Or we do think that configuration space regions should come from a different resource declared in the ACPI namespace if the regions are not MCFG/ECAM compliant (ie config space is not defined through MCFG at all - possibly through a _CRS method for a vendor specific _HID under the PNP0A03 node ?)Hi Lorenzo, For X-Gene: the ECAM space is used to access the configuration space of PCIe devices, with additional help from controller register to specify the bus, device and function number. Below is the RFC patch that implements ECAM fixup for X-Gene PCIe controller on top of this RFC ECAM quirk v3 for your and others reference.
Yes, you have an additional resource in the PNP0A03 _CRS to describe your register that is a deliberate abuse of the ACPI standard in that the _CRS is meant to describe resources that are passed on to secondary buses
quoted hunk
--- From 7a72c122e06aab024497c90fb31790110bebb666 Mon Sep 17 00:00:00 2001 From: Duc Dang <redacted> Date: Wed, 4 May 2016 09:54:00 -0700 Subject: [PATCH 1/1] pci: xgene: ECAM fix-up code for X-Gene PCIe X-Gene PCIe controller does not fully support ECAM. This patch adds required ECAM fixup to allow X-Gene PCIe controller to be functional in ACPI boot mode. Signed-off-by: Duc Dang <redacted> --- drivers/pci/host/Makefile | 2 +- drivers/pci/host/pci-xgene-ecam.c | 198 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 199 insertions(+), 1 deletion(-) create mode 100644 drivers/pci/host/pci-xgene-ecam.cdiff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile index 9c8698e..3480696 100644 --- a/drivers/pci/host/Makefile +++ b/drivers/pci/host/Makefile@@ -14,7 +14,7 @@ obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o obj-$(CONFIG_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o -obj-$(CONFIG_PCI_XGENE) += pci-xgene.o +obj-$(CONFIG_PCI_XGENE) += pci-xgene.o pci-xgene-ecam.o obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.odiff --git a/drivers/pci/host/pci-xgene-ecam.cb/drivers/pci/host/pci-xgene-ecam.c new file mode 100644 index 0000000..253a5c5--- /dev/null +++ b/drivers/pci/host/pci-xgene-ecam.c@@ -0,0 +1,198 @@ +/* + * APM X-Gene PCIe ECAM fixup driver + * + * Copyright (c) 2016, Applied Micro Circuits Corporation + * Author: + * Duc Dang <dhdang@apm.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/of_pci.h> +#include <linux/pci-acpi.h> +#include <linux/platform_device.h> +#include <linux/pci-ecam.h> + +#ifdef CONFIG_ACPI +#define RTDID 0x160 +#define ROOT_CAP_AND_CTRL 0x5C + +/* PCIe IP version */ +#define XGENE_PCIE_IP_VER_UNKN 0 +#define XGENE_PCIE_IP_VER_1 1 + +#define APM_OEM_ID "APM" +#define APM_XGENE_OEM_TABLE_ID "XGENE" +#define APM_XGENE_OEM_REV2 0x00000002 +#define APM_XGENE_OEM_REV1 0x00000001 + +struct xgene_pcie_acpi_root { + void __iomem *csr_base; + u32 version; +}; + +static acpi_status xgene_pcie_find_csr_base(struct acpi_resource *acpi_res, + void *data) +{ + struct xgene_pcie_acpi_root *root = data; + struct acpi_resource_fixed_memory32 *fixed32; + + if (acpi_res->type == ACPI_RESOURCE_TYPE_FIXED_MEMORY32) { + fixed32 = &acpi_res->data.fixed_memory32; + root->csr_base = ioremap(fixed32->address, + fixed32->address_length); + return AE_CTRL_TERMINATE;
This looks completely wrong to me. IIUC you declare a FIXED_MEMORY32 in the PNP0A03 node _CRS which is *not* meant to be used for that, at all. I wonder how this even works: "The _CRS descriptor informs the operating system of the resources it may use for configuring devices below the Host Bridge". AFAICS current code may even end up assigning your FIXED_MEMORY32 resource to bridges/devices (given that it is seen as a memory resource belonging to the PCI host bridge) downstream, how is this supposed to work ? Thanks, Lorenzo
+ }
+
+ return AE_OK;
+}
+
+static int xgene_pcie_ecam_init(struct pci_config_window *cfg)
+{
+ struct xgene_pcie_acpi_root *xgene_root;
+ struct device *dev = cfg->parent;
+ struct acpi_device *adev = to_acpi_device(dev);
+ acpi_handle handle = acpi_device_handle(adev);
+
+ xgene_root = devm_kzalloc(dev, sizeof(*xgene_root), GFP_KERNEL);
+ if (!xgene_root)
+ return -ENOMEM;
+
+ acpi_walk_resources(handle, METHOD_NAME__CRS,
+ xgene_pcie_find_csr_base, xgene_root);
+
+ if (!xgene_root->csr_base) {
+ kfree(xgene_root);
+ return -ENODEV;
+ }
+
+ xgene_root->version = XGENE_PCIE_IP_VER_1;
+
+ cfg->priv = xgene_root;
+
+ return 0;
+}
+
+/*
+ * For Configuration request, RTDID register is used as Bus Number,
+ * Device Number and Function number of the header fields.
+ */
+static void xgene_pcie_set_rtdid_reg(struct pci_bus *bus, uint devfn)
+{
+ struct pci_config_window *cfg = bus->sysdata;
+ struct xgene_pcie_acpi_root *port = cfg->priv;
+ unsigned int b, d, f;
+ u32 rtdid_val = 0;
+
+ b = bus->number;
+ d = PCI_SLOT(devfn);
+ f = PCI_FUNC(devfn);
+
+ if (!pci_is_root_bus(bus))
+ rtdid_val = (b << 8) | (d << 3) | f;
+
+ writel(rtdid_val, port->csr_base + RTDID);
+ /* read the register back to ensure flush */
+ readl(port->csr_base + RTDID);
+}
+
+/*
+ * X-Gene PCIe port uses BAR0-BAR1 of RC's configuration space as
+ * the translation from PCI bus to native BUS. Entire DDR region
+ * is mapped into PCIe space using these registers, so it can be
+ * reached by DMA from EP devices. The BAR0/1 of bridge should be
+ * hidden during enumeration to avoid the sizing and resource allocation
+ * by PCIe core.
+ */
+static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
+{
+ if (pci_is_root_bus(bus) && ((offset == PCI_BASE_ADDRESS_0) ||
+ (offset == PCI_BASE_ADDRESS_1)))
+ return true;
+
+ return false;
+}
+
+void __iomem *xgene_pcie_ecam_map_bus(struct pci_bus *bus,
+ unsigned int devfn, int where)
+{
+ struct pci_config_window *cfg = bus->sysdata;
+ unsigned int busn = bus->number;
+ void __iomem *base;
+
+ if (busn < cfg->busr.start || busn > cfg->busr.end)
+ return NULL;
+
+ if ((pci_is_root_bus(bus) && devfn != 0) ||
+ xgene_pcie_hide_rc_bars(bus, where))
+ return NULL;
+
+ xgene_pcie_set_rtdid_reg(bus, devfn);
+
+ if (busn > cfg->busr.start)
+ base = cfg->win + (1 << cfg->ops->bus_shift);
+ else
+ base = cfg->win;
+
+ return base + where;
+}
+
+static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
+ int where, int size, u32 *val)
+{
+ struct pci_config_window *cfg = bus->sysdata;
+ struct xgene_pcie_acpi_root *port = cfg->priv;
+
+ if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
+ PCIBIOS_SUCCESSFUL)
+ return PCIBIOS_DEVICE_NOT_FOUND;
+
+ /*
+ * The v1 controller has a bug in its Configuration Request
+ * Retry Status (CRS) logic: when CRS is enabled and we read the
+ * Vendor and Device ID of a non-existent device, the controller
+ * fabricates return data of 0xFFFF0001 ("device exists but is not
+ * ready") instead of 0xFFFFFFFF ("device does not exist"). This
+ * causes the PCI core to retry the read until it times out.
+ * Avoid this by not claiming to support CRS.
+ */
+ if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
+ ((where & ~0x3) == ROOT_CAP_AND_CTRL))
+ *val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
+
+ if (size <= 2)
+ *val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
+
+ return PCIBIOS_SUCCESSFUL;
+}
+
+static struct pci_ecam_ops xgene_pcie_ecam_ops = {
+ .bus_shift = 16,
+ .init = xgene_pcie_ecam_init,
+ .pci_ops = {
+ .map_bus = xgene_pcie_ecam_map_bus,
+ .read = xgene_pcie_config_read32,
+ .write = pci_generic_config_write,
+ }
+};
+
+DECLARE_ACPI_MCFG_FIXUP(&xgene_pcie_ecam_ops, APM_OEM_ID,
+ APM_XGENE_OEM_TABLE_ID, APM_XGENE_OEM_REV2,
+ PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
+DECLARE_ACPI_MCFG_FIXUP(&xgene_pcie_ecam_ops, APM_OEM_ID,
+ APM_XGENE_OEM_TABLE_ID, APM_XGENE_OEM_REV1,
+ PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
+#endif
--
1.9.1