* Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
@ 2016-06-17 21:37 Duc Dang
2016-06-20 9:42 ` Lorenzo Pieralisi
0 siblings, 1 reply; 21+ messages in thread
From: Duc Dang @ 2016-06-17 21:37 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Christopher Covington, Tomasz Nowicki, Dongdong Liu, Sinan Kaya,
Jeff Hugo, Gabriele Paoloni, Jon Masters, Mark Salter,
Suravee Suthikulpanit, Jayachandran C, David Daney,
Robert Richter, Hanjun Guo, linux-arm, Catalin Marinas,
Will Deacon, Bjorn Helgaas, Ganapatrao Kulkarni,
Linux Kernel Mailing List
On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>> From: Tomasz Nowicki <tn@semihalf.com>
>>
>> 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 <tn@semihalf.com>
>> ---
>> 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.
---
>From 7a72c122e06aab024497c90fb31790110bebb666 Mon Sep 17 00:00:00 2001
From: Duc Dang <dhdang@apm.com>
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 <dhdang@apm.com>
---
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.c
diff --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.o
diff --git a/drivers/pci/host/pci-xgene-ecam.c
b/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;
+ }
+
+ 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
2016-06-17 21:37 [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller Duc Dang
@ 2016-06-20 9:42 ` Lorenzo Pieralisi
0 siblings, 0 replies; 21+ messages in thread
From: Lorenzo Pieralisi @ 2016-06-20 9:42 UTC (permalink / raw)
To: Duc Dang
Cc: Christopher Covington, Tomasz Nowicki, Dongdong Liu, Sinan Kaya,
Jeff Hugo, Gabriele Paoloni, Jon Masters, Mark Salter,
Suravee Suthikulpanit, Jayachandran C, David Daney,
Robert Richter, Hanjun Guo, linux-arm, Catalin Marinas,
Will Deacon, Bjorn Helgaas, Ganapatrao Kulkarni,
Linux Kernel Mailing List
On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> >> From: Tomasz Nowicki <tn@semihalf.com>
> >>
> >> 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 <tn@semihalf.com>
> >> ---
> >> 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
>
> ---
> From 7a72c122e06aab024497c90fb31790110bebb666 Mon Sep 17 00:00:00 2001
> From: Duc Dang <dhdang@apm.com>
> 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 <dhdang@apm.com>
> ---
> 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.c
>
> diff --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.o
> diff --git a/drivers/pci/host/pci-xgene-ecam.c
> b/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
>
^ permalink raw reply [flat|nested] 21+ messages in thread* [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
@ 2016-06-20 9:42 ` Lorenzo Pieralisi
0 siblings, 0 replies; 21+ messages in thread
From: Lorenzo Pieralisi @ 2016-06-20 9:42 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> >> From: Tomasz Nowicki <tn@semihalf.com>
> >>
> >> 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 <tn@semihalf.com>
> >> ---
> >> 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
>
> ---
> From 7a72c122e06aab024497c90fb31790110bebb666 Mon Sep 17 00:00:00 2001
> From: Duc Dang <dhdang@apm.com>
> 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 <dhdang@apm.com>
> ---
> 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.c
>
> diff --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.o
> diff --git a/drivers/pci/host/pci-xgene-ecam.c
> b/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
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
2016-06-20 9:42 ` Lorenzo Pieralisi
@ 2016-06-20 17:17 ` Christopher Covington
-1 siblings, 0 replies; 21+ messages in thread
From: Christopher Covington @ 2016-06-20 17:17 UTC (permalink / raw)
To: Lorenzo Pieralisi, Duc Dang
Cc: Tomasz Nowicki, Dongdong Liu, Sinan Kaya, Jeff Hugo,
Gabriele Paoloni, Jon Masters, Mark Salter,
Suravee Suthikulpanit, Jayachandran C, David Daney,
Robert Richter, Hanjun Guo, linux-arm, Catalin Marinas,
Will Deacon, Bjorn Helgaas, Ganapatrao Kulkarni,
Linux Kernel Mailing List
Hi Duc,
On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>>> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>>>> From: Tomasz Nowicki <tn@semihalf.com>
>>>>
>>>> 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 <tn@semihalf.com>
>>>> ---
>>>> 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
A potential alternative came up in an off-list discussion: Would it be
better to hard code the information in the quirk workaround than look it
up from a repurposed ACPI resource?
Supporting quirk workarounds for early, non-compliant hardware is
helpful and perhaps necessary for bootstrapping the ecosystem in a
timely manner. But we don't really want to provide an expandable or
reusable interface that would make it easy for new hardware to remain
non-compliant.
Regards,
Cov
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
@ 2016-06-20 17:17 ` Christopher Covington
0 siblings, 0 replies; 21+ messages in thread
From: Christopher Covington @ 2016-06-20 17:17 UTC (permalink / raw)
To: linux-arm-kernel
Hi Duc,
On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>>> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>>>> From: Tomasz Nowicki <tn@semihalf.com>
>>>>
>>>> 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 <tn@semihalf.com>
>>>> ---
>>>> 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
A potential alternative came up in an off-list discussion: Would it be
better to hard code the information in the quirk workaround than look it
up from a repurposed ACPI resource?
Supporting quirk workarounds for early, non-compliant hardware is
helpful and perhaps necessary for bootstrapping the ecosystem in a
timely manner. But we don't really want to provide an expandable or
reusable interface that would make it easy for new hardware to remain
non-compliant.
Regards,
Cov
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
2016-06-20 17:17 ` Christopher Covington
@ 2016-06-20 19:12 ` Duc Dang
-1 siblings, 0 replies; 21+ messages in thread
From: Duc Dang @ 2016-06-20 19:12 UTC (permalink / raw)
To: Christopher Covington
Cc: Lorenzo Pieralisi, Tomasz Nowicki, Dongdong Liu, Sinan Kaya,
Jeff Hugo, Gabriele Paoloni, Jon Masters, Mark Salter,
Suravee Suthikulpanit, Jayachandran C, David Daney,
Robert Richter, Hanjun Guo, linux-arm, Catalin Marinas,
Will Deacon, Bjorn Helgaas, Ganapatrao Kulkarni,
Linux Kernel Mailing List
On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
<cov@codeaurora.org> wrote:
> Hi Duc,
>
> On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
>> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
>>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>>> <lorenzo.pieralisi@arm.com> wrote:
>>>> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>>>>> From: Tomasz Nowicki <tn@semihalf.com>
>>>>>
>>>>> 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 <tn@semihalf.com>
>>>>> ---
>>>>> 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
>
> A potential alternative came up in an off-list discussion: Would it be
> better to hard code the information in the quirk workaround than look it
> up from a repurposed ACPI resource?
Hi Chris, Lorenzo,
Thanks for looking into this.
Yes, I am open for this approach and I think it may work. I can
+ check the pci_config_window resource start address (cfg->res.start)
to figure out the controller and then get the fixed controller
register address
or
+ using the domain number to identify the controller.
Regards,
Duc Dang.
>
> Supporting quirk workarounds for early, non-compliant hardware is
> helpful and perhaps necessary for bootstrapping the ecosystem in a
> timely manner. But we don't really want to provide an expandable or
> reusable interface that would make it easy for new hardware to remain
> non-compliant.
>
> Regards,
> Cov
>
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
@ 2016-06-20 19:12 ` Duc Dang
0 siblings, 0 replies; 21+ messages in thread
From: Duc Dang @ 2016-06-20 19:12 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
<cov@codeaurora.org> wrote:
> Hi Duc,
>
> On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
>> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
>>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>>> <lorenzo.pieralisi@arm.com> wrote:
>>>> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>>>>> From: Tomasz Nowicki <tn@semihalf.com>
>>>>>
>>>>> 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 <tn@semihalf.com>
>>>>> ---
>>>>> 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
>
> A potential alternative came up in an off-list discussion: Would it be
> better to hard code the information in the quirk workaround than look it
> up from a repurposed ACPI resource?
Hi Chris, Lorenzo,
Thanks for looking into this.
Yes, I am open for this approach and I think it may work. I can
+ check the pci_config_window resource start address (cfg->res.start)
to figure out the controller and then get the fixed controller
register address
or
+ using the domain number to identify the controller.
Regards,
Duc Dang.
>
> Supporting quirk workarounds for early, non-compliant hardware is
> helpful and perhaps necessary for bootstrapping the ecosystem in a
> timely manner. But we don't really want to provide an expandable or
> reusable interface that would make it easy for new hardware to remain
> non-compliant.
>
> Regards,
> Cov
>
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
2016-06-20 19:12 ` Duc Dang
@ 2016-06-21 8:42 ` Duc Dang
-1 siblings, 0 replies; 21+ messages in thread
From: Duc Dang @ 2016-06-21 8:42 UTC (permalink / raw)
To: Christopher Covington
Cc: Lorenzo Pieralisi, Tomasz Nowicki, Dongdong Liu, Sinan Kaya,
Jeff Hugo, Gabriele Paoloni, Jon Masters, Mark Salter,
Suravee Suthikulpanit, Jayachandran C, David Daney,
Robert Richter, Hanjun Guo, linux-arm, Catalin Marinas,
Will Deacon, Bjorn Helgaas, Ganapatrao Kulkarni,
Linux Kernel Mailing List
On Mon, Jun 20, 2016 at 12:12 PM, Duc Dang <dhdang@apm.com> wrote:
> On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
> <cov@codeaurora.org> wrote:
>> Hi Duc,
>>
>> On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
>>> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
>>>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>>>> <lorenzo.pieralisi@arm.com> wrote:
>>>>> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>>>>>> From: Tomasz Nowicki <tn@semihalf.com>
>>>>>>
>>>>>> 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 <tn@semihalf.com>
>>>>>> ---
>>>>>> 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
>>
>> A potential alternative came up in an off-list discussion: Would it be
>> better to hard code the information in the quirk workaround than look it
>> up from a repurposed ACPI resource?
>
> Hi Chris, Lorenzo,
>
> Thanks for looking into this.
>
> Yes, I am open for this approach and I think it may work. I can
> + check the pci_config_window resource start address (cfg->res.start)
> to figure out the controller and then get the fixed controller
> register address
> or
> + using the domain number to identify the controller.
Hi Chris, Lorenzo,
I reworked the patch to use fix-ed controller address. Please let me know
if you have further comment/concern.
---
[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 <dhdang@apm.com>
---
v2 changes:
- Using ECAM base address to identify fixed controller address
drivers/pci/host/Makefile | 2 +-
drivers/pci/host/pci-xgene-ecam.c | 291 ++++++++++++++++++++++++++++++++++++++
2 files changed, 292 insertions(+), 1 deletion(-)
create mode 100644 drivers/pci/host/pci-xgene-ecam.c
diff --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.o
diff --git a/drivers/pci/host/pci-xgene-ecam.c
b/drivers/pci/host/pci-xgene-ecam.c
new file mode 100644
index 0000000..87d2dcf
--- /dev/null
+++ b/drivers/pci/host/pci-xgene-ecam.c
@@ -0,0 +1,291 @@
+/*
+ * 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 XGENE_PCIE_IP_VER_2 2
+
+#define XGENE_CSR_LENGTH 0x10000
+
+struct xgene_pcie_acpi_root {
+ void __iomem *csr_base;
+ u32 version;
+};
+
+static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
+{
+ struct xgene_pcie_acpi_root *xgene_root;
+ struct device *dev = cfg->parent;
+ u32 csr_base;
+
+ xgene_root = devm_kzalloc(dev, sizeof(*xgene_root), GFP_KERNEL);
+ if (!xgene_root)
+ return -ENOMEM;
+
+ switch (cfg->res.start) {
+ case 0xE0D0000000ULL:
+ csr_base = 0x1F2B0000;
+ break;
+ case 0xD0D0000000ULL:
+ csr_base = 0x1F2C0000;
+ break;
+ case 0x90D0000000ULL:
+ csr_base = 0x1F2D0000;
+ break;
+ case 0xA0D0000000ULL:
+ csr_base = 0x1F500000;
+ break;
+ case 0xC0D0000000ULL:
+ csr_base = 0x1F510000;
+ break;
+ default:
+ return -ENODEV;
+ }
+
+ xgene_root->csr_base = ioremap(csr_base, XGENE_CSR_LENGTH);
+ if (!xgene_root->csr_base) {
+ kfree(xgene_root);
+ return -ENODEV;
+ }
+
+ xgene_root->version = XGENE_PCIE_IP_VER_1;
+
+ cfg->priv = xgene_root;
+
+ return 0;
+}
+
+static int xgene_v2_1_pcie_ecam_init(struct pci_config_window *cfg)
+{
+ struct xgene_pcie_acpi_root *xgene_root;
+ struct device *dev = cfg->parent;
+ resource_size_t csr_base;
+
+ xgene_root = devm_kzalloc(dev, sizeof(*xgene_root), GFP_KERNEL);
+ if (!xgene_root)
+ return -ENOMEM;
+
+ switch (cfg->res.start) {
+ case 0xC0D0000000ULL:
+ csr_base = 0x1F2B0000;
+ break;
+ case 0xA0D0000000ULL:
+ csr_base = 0x1F2C0000;
+ break;
+ default:
+ return -ENODEV;
+ }
+
+ xgene_root->csr_base = ioremap(csr_base, XGENE_CSR_LENGTH);
+ if (!xgene_root->csr_base) {
+ kfree(xgene_root);
+ return -ENODEV;
+ }
+
+ xgene_root->version = XGENE_PCIE_IP_VER_2;
+
+ cfg->priv = xgene_root;
+
+ return 0;
+}
+
+static int xgene_v2_2_pcie_ecam_init(struct pci_config_window *cfg)
+{
+ struct xgene_pcie_acpi_root *xgene_root;
+ struct device *dev = cfg->parent;
+ resource_size_t csr_base;
+
+ xgene_root = devm_kzalloc(dev, sizeof(*xgene_root), GFP_KERNEL);
+ if (!xgene_root)
+ return -ENOMEM;
+
+ switch (cfg->res.start) {
+ case 0xE0D0000000ULL:
+ csr_base = 0x1F2B0000;
+ break;
+ case 0xA0D0000000ULL:
+ csr_base = 0x1F500000;
+ break;
+ case 0x90D0000000ULL:
+ csr_base = 0x1F2D0000;
+ break;
+ default:
+ return -ENODEV;
+ }
+
+ xgene_root->csr_base = ioremap(csr_base, XGENE_CSR_LENGTH);
+ if (!xgene_root->csr_base) {
+ kfree(xgene_root);
+ return -ENODEV;
+ }
+
+ xgene_root->version = XGENE_PCIE_IP_VER_2;
+
+ 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_v1_pcie_ecam_ops = {
+ .bus_shift = 16,
+ .init = xgene_v1_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_v1_pcie_ecam_ops, "APM", "XGENE", 0x2,
+ PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
+DECLARE_ACPI_MCFG_FIXUP(&xgene_v1_pcie_ecam_ops, "APM", "XGENE", 0x1,
+ PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
+
+static struct pci_ecam_ops xgene_v2_1_pcie_ecam_ops = {
+ .bus_shift = 16,
+ .init = xgene_v2_1_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_v2_1_pcie_ecam_ops, "APM", "XGENE2", 0x1,
+ PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
+
+static struct pci_ecam_ops xgene_v2_2_pcie_ecam_ops = {
+ .bus_shift = 16,
+ .init = xgene_v2_2_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_v2_2_pcie_ecam_ops, "APM", "XGENE2", 0x2,
+ PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
+#endif
--
1.9.1
>>
>> Supporting quirk workarounds for early, non-compliant hardware is
>> helpful and perhaps necessary for bootstrapping the ecosystem in a
>> timely manner. But we don't really want to provide an expandable or
>> reusable interface that would make it easy for new hardware to remain
>> non-compliant.
>>
>> Regards,
>> Cov
>>
>> --
>> Qualcomm Innovation Center, Inc.
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> a Linux Foundation Collaborative Project
Regards,
Duc Dang.
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
@ 2016-06-21 8:42 ` Duc Dang
0 siblings, 0 replies; 21+ messages in thread
From: Duc Dang @ 2016-06-21 8:42 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jun 20, 2016 at 12:12 PM, Duc Dang <dhdang@apm.com> wrote:
> On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
> <cov@codeaurora.org> wrote:
>> Hi Duc,
>>
>> On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
>>> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
>>>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>>>> <lorenzo.pieralisi@arm.com> wrote:
>>>>> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>>>>>> From: Tomasz Nowicki <tn@semihalf.com>
>>>>>>
>>>>>> 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 <tn@semihalf.com>
>>>>>> ---
>>>>>> 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
>>
>> A potential alternative came up in an off-list discussion: Would it be
>> better to hard code the information in the quirk workaround than look it
>> up from a repurposed ACPI resource?
>
> Hi Chris, Lorenzo,
>
> Thanks for looking into this.
>
> Yes, I am open for this approach and I think it may work. I can
> + check the pci_config_window resource start address (cfg->res.start)
> to figure out the controller and then get the fixed controller
> register address
> or
> + using the domain number to identify the controller.
Hi Chris, Lorenzo,
I reworked the patch to use fix-ed controller address. Please let me know
if you have further comment/concern.
---
[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 <dhdang@apm.com>
---
v2 changes:
- Using ECAM base address to identify fixed controller address
drivers/pci/host/Makefile | 2 +-
drivers/pci/host/pci-xgene-ecam.c | 291 ++++++++++++++++++++++++++++++++++++++
2 files changed, 292 insertions(+), 1 deletion(-)
create mode 100644 drivers/pci/host/pci-xgene-ecam.c
diff --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.o
diff --git a/drivers/pci/host/pci-xgene-ecam.c
b/drivers/pci/host/pci-xgene-ecam.c
new file mode 100644
index 0000000..87d2dcf
--- /dev/null
+++ b/drivers/pci/host/pci-xgene-ecam.c
@@ -0,0 +1,291 @@
+/*
+ * 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 XGENE_PCIE_IP_VER_2 2
+
+#define XGENE_CSR_LENGTH 0x10000
+
+struct xgene_pcie_acpi_root {
+ void __iomem *csr_base;
+ u32 version;
+};
+
+static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
+{
+ struct xgene_pcie_acpi_root *xgene_root;
+ struct device *dev = cfg->parent;
+ u32 csr_base;
+
+ xgene_root = devm_kzalloc(dev, sizeof(*xgene_root), GFP_KERNEL);
+ if (!xgene_root)
+ return -ENOMEM;
+
+ switch (cfg->res.start) {
+ case 0xE0D0000000ULL:
+ csr_base = 0x1F2B0000;
+ break;
+ case 0xD0D0000000ULL:
+ csr_base = 0x1F2C0000;
+ break;
+ case 0x90D0000000ULL:
+ csr_base = 0x1F2D0000;
+ break;
+ case 0xA0D0000000ULL:
+ csr_base = 0x1F500000;
+ break;
+ case 0xC0D0000000ULL:
+ csr_base = 0x1F510000;
+ break;
+ default:
+ return -ENODEV;
+ }
+
+ xgene_root->csr_base = ioremap(csr_base, XGENE_CSR_LENGTH);
+ if (!xgene_root->csr_base) {
+ kfree(xgene_root);
+ return -ENODEV;
+ }
+
+ xgene_root->version = XGENE_PCIE_IP_VER_1;
+
+ cfg->priv = xgene_root;
+
+ return 0;
+}
+
+static int xgene_v2_1_pcie_ecam_init(struct pci_config_window *cfg)
+{
+ struct xgene_pcie_acpi_root *xgene_root;
+ struct device *dev = cfg->parent;
+ resource_size_t csr_base;
+
+ xgene_root = devm_kzalloc(dev, sizeof(*xgene_root), GFP_KERNEL);
+ if (!xgene_root)
+ return -ENOMEM;
+
+ switch (cfg->res.start) {
+ case 0xC0D0000000ULL:
+ csr_base = 0x1F2B0000;
+ break;
+ case 0xA0D0000000ULL:
+ csr_base = 0x1F2C0000;
+ break;
+ default:
+ return -ENODEV;
+ }
+
+ xgene_root->csr_base = ioremap(csr_base, XGENE_CSR_LENGTH);
+ if (!xgene_root->csr_base) {
+ kfree(xgene_root);
+ return -ENODEV;
+ }
+
+ xgene_root->version = XGENE_PCIE_IP_VER_2;
+
+ cfg->priv = xgene_root;
+
+ return 0;
+}
+
+static int xgene_v2_2_pcie_ecam_init(struct pci_config_window *cfg)
+{
+ struct xgene_pcie_acpi_root *xgene_root;
+ struct device *dev = cfg->parent;
+ resource_size_t csr_base;
+
+ xgene_root = devm_kzalloc(dev, sizeof(*xgene_root), GFP_KERNEL);
+ if (!xgene_root)
+ return -ENOMEM;
+
+ switch (cfg->res.start) {
+ case 0xE0D0000000ULL:
+ csr_base = 0x1F2B0000;
+ break;
+ case 0xA0D0000000ULL:
+ csr_base = 0x1F500000;
+ break;
+ case 0x90D0000000ULL:
+ csr_base = 0x1F2D0000;
+ break;
+ default:
+ return -ENODEV;
+ }
+
+ xgene_root->csr_base = ioremap(csr_base, XGENE_CSR_LENGTH);
+ if (!xgene_root->csr_base) {
+ kfree(xgene_root);
+ return -ENODEV;
+ }
+
+ xgene_root->version = XGENE_PCIE_IP_VER_2;
+
+ 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_v1_pcie_ecam_ops = {
+ .bus_shift = 16,
+ .init = xgene_v1_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_v1_pcie_ecam_ops, "APM", "XGENE", 0x2,
+ PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
+DECLARE_ACPI_MCFG_FIXUP(&xgene_v1_pcie_ecam_ops, "APM", "XGENE", 0x1,
+ PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
+
+static struct pci_ecam_ops xgene_v2_1_pcie_ecam_ops = {
+ .bus_shift = 16,
+ .init = xgene_v2_1_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_v2_1_pcie_ecam_ops, "APM", "XGENE2", 0x1,
+ PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
+
+static struct pci_ecam_ops xgene_v2_2_pcie_ecam_ops = {
+ .bus_shift = 16,
+ .init = xgene_v2_2_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_v2_2_pcie_ecam_ops, "APM", "XGENE2", 0x2,
+ PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
+#endif
--
1.9.1
>>
>> Supporting quirk workarounds for early, non-compliant hardware is
>> helpful and perhaps necessary for bootstrapping the ecosystem in a
>> timely manner. But we don't really want to provide an expandable or
>> reusable interface that would make it easy for new hardware to remain
>> non-compliant.
>>
>> Regards,
>> Cov
>>
>> --
>> Qualcomm Innovation Center, Inc.
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> a Linux Foundation Collaborative Project
Regards,
Duc Dang.
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
2016-06-20 19:12 ` Duc Dang
@ 2016-06-21 9:26 ` Lorenzo Pieralisi
-1 siblings, 0 replies; 21+ messages in thread
From: Lorenzo Pieralisi @ 2016-06-21 9:26 UTC (permalink / raw)
To: Duc Dang
Cc: Christopher Covington, Tomasz Nowicki, Dongdong Liu, Sinan Kaya,
Jeff Hugo, Gabriele Paoloni, Jon Masters, Mark Salter,
Suravee Suthikulpanit, Jayachandran C, David Daney,
Robert Richter, Hanjun Guo, linux-arm, Catalin Marinas,
Will Deacon, Bjorn Helgaas, Ganapatrao Kulkarni,
Linux Kernel Mailing List
On Mon, Jun 20, 2016 at 12:12:24PM -0700, Duc Dang wrote:
> On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
> <cov@codeaurora.org> wrote:
> > Hi Duc,
> >
> > On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
> >> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
> >>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
> >>> <lorenzo.pieralisi@arm.com> wrote:
> >>>> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> >>>>> From: Tomasz Nowicki <tn@semihalf.com>
> >>>>>
> >>>>> 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 <tn@semihalf.com>
> >>>>> ---
> >>>>> 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
> >
> > A potential alternative came up in an off-list discussion: Would it be
> > better to hard code the information in the quirk workaround than look it
> > up from a repurposed ACPI resource?
>
> Hi Chris, Lorenzo,
>
> Thanks for looking into this.
>
> Yes, I am open for this approach and I think it may work. I can +
> check the pci_config_window resource start address (cfg->res.start) to
> figure out the controller and then get the fixed controller register
> address or + using the domain number to identify the controller.
First thing to do is to remove config space entry from PNP0A03
_CRS, that's a FW bug.
You could use the {bus-range, domain} to get that register address,
anyway, that's not the main concern here.
Thanks,
Lorenzo
> Regards,
> Duc Dang.
> >
> > Supporting quirk workarounds for early, non-compliant hardware is
> > helpful and perhaps necessary for bootstrapping the ecosystem in a
> > timely manner. But we don't really want to provide an expandable or
> > reusable interface that would make it easy for new hardware to remain
> > non-compliant.
> >
> > Regards,
> > Cov
> >
> > --
> > Qualcomm Innovation Center, Inc.
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > a Linux Foundation Collaborative Project
>
^ permalink raw reply [flat|nested] 21+ messages in thread* [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
@ 2016-06-21 9:26 ` Lorenzo Pieralisi
0 siblings, 0 replies; 21+ messages in thread
From: Lorenzo Pieralisi @ 2016-06-21 9:26 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jun 20, 2016 at 12:12:24PM -0700, Duc Dang wrote:
> On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
> <cov@codeaurora.org> wrote:
> > Hi Duc,
> >
> > On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
> >> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
> >>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
> >>> <lorenzo.pieralisi@arm.com> wrote:
> >>>> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> >>>>> From: Tomasz Nowicki <tn@semihalf.com>
> >>>>>
> >>>>> 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 <tn@semihalf.com>
> >>>>> ---
> >>>>> 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
> >
> > A potential alternative came up in an off-list discussion: Would it be
> > better to hard code the information in the quirk workaround than look it
> > up from a repurposed ACPI resource?
>
> Hi Chris, Lorenzo,
>
> Thanks for looking into this.
>
> Yes, I am open for this approach and I think it may work. I can +
> check the pci_config_window resource start address (cfg->res.start) to
> figure out the controller and then get the fixed controller register
> address or + using the domain number to identify the controller.
First thing to do is to remove config space entry from PNP0A03
_CRS, that's a FW bug.
You could use the {bus-range, domain} to get that register address,
anyway, that's not the main concern here.
Thanks,
Lorenzo
> Regards,
> Duc Dang.
> >
> > Supporting quirk workarounds for early, non-compliant hardware is
> > helpful and perhaps necessary for bootstrapping the ecosystem in a
> > timely manner. But we don't really want to provide an expandable or
> > reusable interface that would make it easy for new hardware to remain
> > non-compliant.
> >
> > Regards,
> > Cov
> >
> > --
> > Qualcomm Innovation Center, Inc.
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > a Linux Foundation Collaborative Project
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
2016-06-21 9:26 ` Lorenzo Pieralisi
@ 2016-06-21 9:29 ` Duc Dang
-1 siblings, 0 replies; 21+ messages in thread
From: Duc Dang @ 2016-06-21 9:29 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Christopher Covington, Tomasz Nowicki, Dongdong Liu, Sinan Kaya,
Jeff Hugo, Gabriele Paoloni, Jon Masters, Mark Salter,
Suravee Suthikulpanit, Jayachandran C, David Daney,
Robert Richter, Hanjun Guo, linux-arm, Catalin Marinas,
Will Deacon, Bjorn Helgaas, Ganapatrao Kulkarni,
Linux Kernel Mailing List
On Tue, Jun 21, 2016 at 2:26 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Mon, Jun 20, 2016 at 12:12:24PM -0700, Duc Dang wrote:
>> On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
>> <cov@codeaurora.org> wrote:
>> > Hi Duc,
>> >
>> > On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
>> >> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
>> >>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>> >>> <lorenzo.pieralisi@arm.com> wrote:
>> >>>> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>> >>>>> From: Tomasz Nowicki <tn@semihalf.com>
>> >>>>>
>> >>>>> 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 <tn@semihalf.com>
>> >>>>> ---
>> >>>>> 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
>> >
>> > A potential alternative came up in an off-list discussion: Would it be
>> > better to hard code the information in the quirk workaround than look it
>> > up from a repurposed ACPI resource?
>>
>> Hi Chris, Lorenzo,
>>
>> Thanks for looking into this.
>>
>> Yes, I am open for this approach and I think it may work. I can +
>> check the pci_config_window resource start address (cfg->res.start) to
>> figure out the controller and then get the fixed controller register
>> address or + using the domain number to identify the controller.
>
> First thing to do is to remove config space entry from PNP0A03
> _CRS, that's a FW bug.
Yes, I will remove it in the next version of firmware release.
>
> You could use the {bus-range, domain} to get that register address,
> anyway, that's not the main concern here.
>
> Thanks,
> Lorenzo
>
>> Regards,
>> Duc Dang.
>> >
>> > Supporting quirk workarounds for early, non-compliant hardware is
>> > helpful and perhaps necessary for bootstrapping the ecosystem in a
>> > timely manner. But we don't really want to provide an expandable or
>> > reusable interface that would make it easy for new hardware to remain
>> > non-compliant.
>> >
>> > Regards,
>> > Cov
>> >
>> > --
>> > Qualcomm Innovation Center, Inc.
>> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> > a Linux Foundation Collaborative Project
>>
Regards,
Duc Dang.
^ permalink raw reply [flat|nested] 21+ messages in thread* [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
@ 2016-06-21 9:29 ` Duc Dang
0 siblings, 0 replies; 21+ messages in thread
From: Duc Dang @ 2016-06-21 9:29 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jun 21, 2016 at 2:26 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Mon, Jun 20, 2016 at 12:12:24PM -0700, Duc Dang wrote:
>> On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
>> <cov@codeaurora.org> wrote:
>> > Hi Duc,
>> >
>> > On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
>> >> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
>> >>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>> >>> <lorenzo.pieralisi@arm.com> wrote:
>> >>>> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>> >>>>> From: Tomasz Nowicki <tn@semihalf.com>
>> >>>>>
>> >>>>> 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 <tn@semihalf.com>
>> >>>>> ---
>> >>>>> 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
>> >
>> > A potential alternative came up in an off-list discussion: Would it be
>> > better to hard code the information in the quirk workaround than look it
>> > up from a repurposed ACPI resource?
>>
>> Hi Chris, Lorenzo,
>>
>> Thanks for looking into this.
>>
>> Yes, I am open for this approach and I think it may work. I can +
>> check the pci_config_window resource start address (cfg->res.start) to
>> figure out the controller and then get the fixed controller register
>> address or + using the domain number to identify the controller.
>
> First thing to do is to remove config space entry from PNP0A03
> _CRS, that's a FW bug.
Yes, I will remove it in the next version of firmware release.
>
> You could use the {bus-range, domain} to get that register address,
> anyway, that's not the main concern here.
>
> Thanks,
> Lorenzo
>
>> Regards,
>> Duc Dang.
>> >
>> > Supporting quirk workarounds for early, non-compliant hardware is
>> > helpful and perhaps necessary for bootstrapping the ecosystem in a
>> > timely manner. But we don't really want to provide an expandable or
>> > reusable interface that would make it easy for new hardware to remain
>> > non-compliant.
>> >
>> > Regards,
>> > Cov
>> >
>> > --
>> > Qualcomm Innovation Center, Inc.
>> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> > a Linux Foundation Collaborative Project
>>
Regards,
Duc Dang.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v3 1/2] ACPI/PCI: Check platform specific ECAM quirks
@ 2016-06-15 15:34 Christopher Covington
2016-06-15 15:34 ` Christopher Covington
0 siblings, 1 reply; 21+ messages in thread
From: Christopher Covington @ 2016-06-15 15:34 UTC (permalink / raw)
To: Tomasz Nowicki, Duc Dang, Dongdong Liu
Cc: Sinan Kaya, Jeff Hugo, Gabriele Paoloni, Jon Masters,
Mark Salter, Suravee Suthikulpanit, Jayachandran C, David Daney,
Robert Richter, Lorenzo Pieralisi, Hanjun Guo, linux-arm-kernel,
Christopher Covington, Rafael J. Wysocki, Len Brown,
Arnd Bergmann, Bjorn Helgaas, linux-acpi, linux-kernel,
linux-arch, linux-pci
From: Tomasz Nowicki <tn@semihalf.com>
Some platforms may not be fully compliant with the generic PCI config
operations. For these cases we implement a way to use custom map and
accessor functions. The algorithm traverses the available quirk list,
matches against <oem_id, oem_table_id, oem_revision, domain, bus number>
tuples and returns corresponding PCI config ops. oem_id, oem_table_id, and
oem_revision come from the MCFG table standard header. All quirks can be
defined using the DECLARE_ACPI_MCFG_FIXUP() macro and are kept self
contained. Example:
/* Custom PCI config ops */
static struct pci_generic_ecam_ops foo_pci_ops = {
.bus_shift = 24,
.pci_ops = {
.map_bus = pci_ecam_map_bus,
.read = foo_ecam_config_read,
.write = foo_ecam_config_write,
}
};
DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, "OEM", "TABLE-ID", <revision>,
<domain_nr>, <bus_nr>);
Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
Signed-off-by: Christopher Covington <cov@codeaurora.org>
---
Changes from v2 to v3:
* Match against all three of oem_id, oem_table_id, and oem_revision.
* Perform substring match, so padding oem_id and oem_table_id isn't
required. (Using min_t() thanks to Duc Dang's suggestion.)
* Print when quirk matched.
* Use __UNIQUE_ID() macro to generate a valid, unique symbol name even
when OEM and table IDs contain characters such as dash "-".
* Move extern declaration to header and fix spelling and formatting to
satisfy checkpatch/coding style.
---
drivers/acpi/pci_mcfg.c | 49 ++++++++++++++++++++++++++++++++++++---
include/asm-generic/vmlinux.lds.h | 7 ++++++
include/linux/pci-acpi.h | 24 +++++++++++++++++++
3 files changed, 77 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index b5b376e..2a5d3dd 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -22,6 +22,10 @@
#include <linux/kernel.h>
#include <linux/pci.h>
#include <linux/pci-acpi.h>
+#include <linux/pci-ecam.h>
+
+/* Root pointer to the mapped MCFG table */
+static struct acpi_table_mcfg *mcfg_table;
/* Structure to hold entries from the MCFG table */
struct mcfg_entry {
@@ -35,6 +39,46 @@ struct mcfg_entry {
/* List to save MCFG entries */
static LIST_HEAD(pci_mcfg_list);
+static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
+ struct acpi_table_header *h)
+{
+ int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
+ int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
+
+ return (!strncmp(f->oem_id, h->oem_id, olen) &&
+ !strncmp(f->oem_table_id, h->oem_table_id, tlen) &&
+ f->oem_revision == h->oem_revision);
+}
+
+struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
+{
+ int bus_num = root->secondary.start;
+ int domain = root->segment;
+ struct pci_cfg_fixup *f;
+
+ if (!mcfg_table)
+ return &pci_generic_ecam_ops;
+
+ /*
+ * Match against platform specific quirks and return corresponding CAM
+ * ops.
+ *
+ * First match against PCI topology <domain:bus> then use OEM ID, OEM
+ * table ID, and OEM revision from MCFG table standard header.
+ */
+ for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
+ if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
+ (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
+ pci_mcfg_fixup_match(f, &mcfg_table->header)) {
+ pr_info("Handling %s %s r%d PCI MCFG quirks\n",
+ f->oem_id, f->oem_table_id, f->oem_revision);
+ return f->ops;
+ }
+ }
+ /* No quirks, use ECAM */
+ return &pci_generic_ecam_ops;
+}
+
phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
{
struct mcfg_entry *e;
@@ -54,7 +98,6 @@ phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
static __init int pci_mcfg_parse(struct acpi_table_header *header)
{
- struct acpi_table_mcfg *mcfg;
struct acpi_mcfg_allocation *mptr;
struct mcfg_entry *e, *arr;
int i, n;
@@ -64,8 +107,8 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
n = (header->length - sizeof(struct acpi_table_mcfg)) /
sizeof(struct acpi_mcfg_allocation);
- mcfg = (struct acpi_table_mcfg *)header;
- mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
+ mcfg_table = (struct acpi_table_mcfg *)header;
+ mptr = (struct acpi_mcfg_allocation *) &mcfg_table[1];
arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
if (!arr)
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 6a67ab9..43604fc 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -300,6 +300,13 @@
VMLINUX_SYMBOL(__end_pci_fixups_suspend_late) = .; \
} \
\
+ /* ACPI MCFG quirks */ \
+ .acpi_fixup : AT(ADDR(.acpi_fixup) - LOAD_OFFSET) { \
+ VMLINUX_SYMBOL(__start_acpi_mcfg_fixups) = .; \
+ *(.acpi_fixup_mcfg) \
+ VMLINUX_SYMBOL(__end_acpi_mcfg_fixups) = .; \
+ } \
+ \
/* Built-in firmware blobs */ \
.builtin_fw : AT(ADDR(.builtin_fw) - LOAD_OFFSET) { \
VMLINUX_SYMBOL(__start_builtin_fw) = .; \
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 7d63a66..dac851b 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -25,6 +25,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource *bus_res);
+extern struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root);
static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
{
@@ -72,6 +73,29 @@ struct acpi_pci_root_ops {
int (*prepare_resources)(struct acpi_pci_root_info *info);
};
+struct pci_cfg_fixup {
+ struct pci_ecam_ops *ops;
+ char *oem_id;
+ char *oem_table_id;
+ u32 oem_revision;
+ int domain;
+ int bus_num;
+};
+
+extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
+extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
+
+#define PCI_MCFG_DOMAIN_ANY -1
+#define PCI_MCFG_BUS_ANY -1
+
+/* Designate a routine to fix up buggy MCFG */
+#define DECLARE_ACPI_MCFG_FIXUP(ops, oem, table, rev, dom, bus) \
+ static const struct pci_cfg_fixup \
+ __UNIQUE_ID(mcfg_fixup_) \
+ __used __attribute__((__section__(".acpi_fixup_mcfg"), \
+ aligned((sizeof(void *))))) = \
+ { ops, oem, table, rev, dom, bus }
+
extern int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info);
extern struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
struct acpi_pci_root_ops *ops,
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
2016-06-15 15:34 [RFC PATCH v3 1/2] ACPI/PCI: Check platform specific ECAM quirks Christopher Covington
@ 2016-06-15 15:34 ` Christopher Covington
0 siblings, 0 replies; 21+ messages in thread
From: Christopher Covington @ 2016-06-15 15:34 UTC (permalink / raw)
To: Tomasz Nowicki, Duc Dang, Dongdong Liu
Cc: Sinan Kaya, Jeff Hugo, Gabriele Paoloni, Jon Masters,
Mark Salter, Suravee Suthikulpanit, Jayachandran C, David Daney,
Robert Richter, Lorenzo Pieralisi, Hanjun Guo, linux-arm-kernel,
Catalin Marinas, Will Deacon, Bjorn Helgaas, Lorenzo Pieralisi,
Christopher Covington, Ganapatrao Kulkarni, linux-kernel
From: Tomasz Nowicki <tn@semihalf.com>
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 <tn@semihalf.com>
---
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);
if (IS_ERR(cfg)) {
dev_err(&root->device->dev, "%04x:%pR error %ld mapping ECAM\n",
seg, bus_res, PTR_ERR(cfg));
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
@ 2016-06-15 15:34 ` Christopher Covington
0 siblings, 0 replies; 21+ messages in thread
From: Christopher Covington @ 2016-06-15 15:34 UTC (permalink / raw)
To: linux-arm-kernel
From: Tomasz Nowicki <tn@semihalf.com>
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 <tn@semihalf.com>
---
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);
if (IS_ERR(cfg)) {
dev_err(&root->device->dev, "%04x:%pR error %ld mapping ECAM\n",
seg, bus_res, PTR_ERR(cfg));
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
2016-06-15 15:34 ` Christopher Covington
@ 2016-06-16 17:48 ` Lorenzo Pieralisi
-1 siblings, 0 replies; 21+ messages in thread
From: Lorenzo Pieralisi @ 2016-06-16 17:48 UTC (permalink / raw)
To: Christopher Covington
Cc: Tomasz Nowicki, Duc Dang, Dongdong Liu, Sinan Kaya, Jeff Hugo,
Gabriele Paoloni, Jon Masters, Mark Salter,
Suravee Suthikulpanit, Jayachandran C, David Daney,
Robert Richter, Hanjun Guo, linux-arm-kernel, Catalin Marinas,
Will Deacon, Bjorn Helgaas, Ganapatrao Kulkarni, linux-kernel
On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> From: Tomasz Nowicki <tn@semihalf.com>
>
> 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 <tn@semihalf.com>
> ---
> 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 ?)
It might even be a choice we do not have anymore, but I think it
is important to make a decision and proceed accordingly.
Comments appreciated.
Thanks,
Lorenzo
> if (IS_ERR(cfg)) {
> dev_err(&root->device->dev, "%04x:%pR error %ld mapping ECAM\n",
> seg, bus_res, PTR_ERR(cfg));
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
^ permalink raw reply [flat|nested] 21+ messages in thread* [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
@ 2016-06-16 17:48 ` Lorenzo Pieralisi
0 siblings, 0 replies; 21+ messages in thread
From: Lorenzo Pieralisi @ 2016-06-16 17:48 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> From: Tomasz Nowicki <tn@semihalf.com>
>
> 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 <tn@semihalf.com>
> ---
> 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 ?)
It might even be a choice we do not have anymore, but I think it
is important to make a decision and proceed accordingly.
Comments appreciated.
Thanks,
Lorenzo
> if (IS_ERR(cfg)) {
> dev_err(&root->device->dev, "%04x:%pR error %ld mapping ECAM\n",
> seg, bus_res, PTR_ERR(cfg));
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
^ permalink raw reply [flat|nested] 21+ messages in thread* RE: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
2016-06-16 17:48 ` Lorenzo Pieralisi
@ 2016-06-17 8:01 ` Gabriele Paoloni
-1 siblings, 0 replies; 21+ messages in thread
From: Gabriele Paoloni @ 2016-06-17 8:01 UTC (permalink / raw)
To: Lorenzo Pieralisi, Christopher Covington
Cc: Tomasz Nowicki, Duc Dang, liudongdong (C),
Sinan Kaya, Jeff Hugo, Jon Masters, Mark Salter,
Suravee Suthikulpanit, Jayachandran C, David Daney,
Robert Richter, Hanjun Guo, linux-arm-kernel, Catalin Marinas,
Will Deacon, Bjorn Helgaas, Ganapatrao Kulkarni, linux-kernel
Hi Lorenzo and All
> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
> Sent: 16 June 2016 18:49
> To: Christopher Covington
> Cc: Tomasz Nowicki; Duc Dang; liudongdong (C); Sinan Kaya; Jeff Hugo;
> Gabriele Paoloni; Jon Masters; Mark Salter; Suravee Suthikulpanit;
> Jayachandran C; David Daney; Robert Richter; Hanjun Guo; linux-arm-
> kernel@lists.infradead.org; Catalin Marinas; Will Deacon; Bjorn Helgaas;
> Ganapatrao Kulkarni; linux-kernel@vger.kernel.org
> Subject: Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling
> for ACPI based PCI host controller
>
> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> > From: Tomasz Nowicki <tn@semihalf.com>
> >
> > 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 <tn@semihalf.com>
> > ---
> > 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 ?
I didn't dig into the other vendors' quirk mechanism but I will
quickly explain what we (would like to) do in HiSilicon Hip05/Hip06
SoCs.
>From our perspective we use ECAM access mechanism for all the MCFG
buses except the root ports.
For the root ports we declare additional ACPI devices marked as
"pnp0c02" (motherboard reserved resource) i.e.:
Scope(_SB)
{
// PCIe Root bus
Device (PCI1)
{
Name (_HID, "HISI0080") // PCI Express Root Bridge
Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
Name(_SEG, 1) // Segment of this Root complex
Name(_BBN, 64) // Base Bus Number
Name(_CCA, 1)
Method (_CRS, 0, Serialized) { // Root complex resources
Name (RBUF, ResourceTemplate () {
[...]
)
}) // Name(RBUF)
Return (RBUF)
} // Method(_CRS)
Device (RES1)
{
Name (_HID, "HISI0081") // HiSi PCIe RC config base address
Name (_CID, "PNP0C02") // Motherboard reserved resource
Name (_CRS, ResourceTemplate (){
Memory32Fixed (ReadWrite, 0xb0080000 , 0x10000)
})
}
[...]
} // Device(PCI1)
Therefore we declare a perfectly ECAM compliant MCFG and we "waste"
the root ports addresses as in practice for the root ports we replace
the MCFG address with the one retrieved from the ACPI device above.
In terms of "safety" I think it should be ok as in practice we are
reserving some addresses in MCFG that we do not use and the ones
that are used are reserved as well in the ACPI namespace.
>From a general perspective it seems to me that pci_mcfg_lookup
can only parse mcfg entries that are declared according to
the standard (i.e. you cannot declare a hacky mcfg table).
Obviously the quirks allow the vendors to declare their own
cfg_rd/wr function and therefore to do anything they want with
the MCFG addresses.
>From my perspective I think the easiest solution is to keep this
quirk mechanism in place and then review vendor by vendor solution
as they are pushed to the mailing list; if some vendors are abusing
of some addresses/resources then they can be rejected...
What do you think?
Thanks
Gab
>
> 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 ?)
>
> It might even be a choice we do not have anymore, but I think it
> is important to make a decision and proceed accordingly.
>
> Comments appreciated.
>
> Thanks,
> Lorenzo
>
> > if (IS_ERR(cfg)) {
> > dev_err(&root->device->dev, "%04x:%pR error %ld mapping
> ECAM\n",
> > seg, bus_res, PTR_ERR(cfg));
> > --
> > Qualcomm Innovation Center, Inc.
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > a Linux Foundation Collaborative Project
> >
^ permalink raw reply [flat|nested] 21+ messages in thread* [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
@ 2016-06-17 8:01 ` Gabriele Paoloni
0 siblings, 0 replies; 21+ messages in thread
From: Gabriele Paoloni @ 2016-06-17 8:01 UTC (permalink / raw)
To: linux-arm-kernel
Hi Lorenzo and All
> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> Sent: 16 June 2016 18:49
> To: Christopher Covington
> Cc: Tomasz Nowicki; Duc Dang; liudongdong (C); Sinan Kaya; Jeff Hugo;
> Gabriele Paoloni; Jon Masters; Mark Salter; Suravee Suthikulpanit;
> Jayachandran C; David Daney; Robert Richter; Hanjun Guo; linux-arm-
> kernel at lists.infradead.org; Catalin Marinas; Will Deacon; Bjorn Helgaas;
> Ganapatrao Kulkarni; linux-kernel at vger.kernel.org
> Subject: Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling
> for ACPI based PCI host controller
>
> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> > From: Tomasz Nowicki <tn@semihalf.com>
> >
> > 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 <tn@semihalf.com>
> > ---
> > 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 ?
I didn't dig into the other vendors' quirk mechanism but I will
quickly explain what we (would like to) do in HiSilicon Hip05/Hip06
SoCs.
>From our perspective we use ECAM access mechanism for all the MCFG
buses except the root ports.
For the root ports we declare additional ACPI devices marked as
"pnp0c02" (motherboard reserved resource) i.e.:
Scope(_SB)
{
// PCIe Root bus
Device (PCI1)
{
Name (_HID, "HISI0080") // PCI Express Root Bridge
Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
Name(_SEG, 1) // Segment of this Root complex
Name(_BBN, 64) // Base Bus Number
Name(_CCA, 1)
Method (_CRS, 0, Serialized) { // Root complex resources
Name (RBUF, ResourceTemplate () {
[...]
)
}) // Name(RBUF)
Return (RBUF)
} // Method(_CRS)
Device (RES1)
{
Name (_HID, "HISI0081") // HiSi PCIe RC config base address
Name (_CID, "PNP0C02") // Motherboard reserved resource
Name (_CRS, ResourceTemplate (){
Memory32Fixed (ReadWrite, 0xb0080000 , 0x10000)
})
}
[...]
} // Device(PCI1)
Therefore we declare a perfectly ECAM compliant MCFG and we "waste"
the root ports addresses as in practice for the root ports we replace
the MCFG address with the one retrieved from the ACPI device above.
In terms of "safety" I think it should be ok as in practice we are
reserving some addresses in MCFG that we do not use and the ones
that are used are reserved as well in the ACPI namespace.
>From a general perspective it seems to me that pci_mcfg_lookup
can only parse mcfg entries that are declared according to
the standard (i.e. you cannot declare a hacky mcfg table).
Obviously the quirks allow the vendors to declare their own
cfg_rd/wr function and therefore to do anything they want with
the MCFG addresses.
>From my perspective I think the easiest solution is to keep this
quirk mechanism in place and then review vendor by vendor solution
as they are pushed to the mailing list; if some vendors are abusing
of some addresses/resources then they can be rejected...
What do you think?
Thanks
Gab
>
> 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 ?)
>
> It might even be a choice we do not have anymore, but I think it
> is important to make a decision and proceed accordingly.
>
> Comments appreciated.
>
> Thanks,
> Lorenzo
>
> > if (IS_ERR(cfg)) {
> > dev_err(&root->device->dev, "%04x:%pR error %ld mapping
> ECAM\n",
> > seg, bus_res, PTR_ERR(cfg));
> > --
> > Qualcomm Innovation Center, Inc.
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > a Linux Foundation Collaborative Project
> >
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
2016-06-17 8:01 ` Gabriele Paoloni
@ 2016-06-17 14:13 ` Christopher Covington
-1 siblings, 0 replies; 21+ messages in thread
From: Christopher Covington @ 2016-06-17 14:13 UTC (permalink / raw)
To: Gabriele Paoloni, Lorenzo Pieralisi
Cc: Tomasz Nowicki, Duc Dang, liudongdong (C),
Sinan Kaya, Jeff Hugo, Jon Masters, Mark Salter,
Suravee Suthikulpanit, Jayachandran C, David Daney,
Robert Richter, Hanjun Guo, linux-arm-kernel, Catalin Marinas,
Will Deacon, Bjorn Helgaas, Ganapatrao Kulkarni, linux-kernel
On 06/17/2016 04:01 AM, Gabriele Paoloni wrote:
> Hi Lorenzo and All
>
>> -----Original Message-----
>> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
>> Sent: 16 June 2016 18:49
>> To: Christopher Covington
>> Cc: Tomasz Nowicki; Duc Dang; liudongdong (C); Sinan Kaya; Jeff Hugo;
>> Gabriele Paoloni; Jon Masters; Mark Salter; Suravee Suthikulpanit;
>> Jayachandran C; David Daney; Robert Richter; Hanjun Guo; linux-arm-
>> kernel@lists.infradead.org; Catalin Marinas; Will Deacon; Bjorn Helgaas;
>> Ganapatrao Kulkarni; linux-kernel@vger.kernel.org
>> Subject: Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling
>> for ACPI based PCI host controller
>>
>> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>>> From: Tomasz Nowicki <tn@semihalf.com>
>>>
>>> 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 <tn@semihalf.com>
>>> ---
>>> 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 ?
> From my perspective I think the easiest solution is to keep this
> quirk mechanism in place and then review vendor by vendor solution
> as they are pushed to the mailing list; if some vendors are abusing
> of some addresses/resources then they can be rejected...
Using pci_ecam_create() works well for QDF2432:
https://us.codeaurora.org/cgit/quic/server/kernel/commit/?h=cov/4.7-rc3-testing&id=ca7439a54ddd8612b435c797ffc54f4e19f03e2b
Cheers,
Cov
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller
@ 2016-06-17 14:13 ` Christopher Covington
0 siblings, 0 replies; 21+ messages in thread
From: Christopher Covington @ 2016-06-17 14:13 UTC (permalink / raw)
To: linux-arm-kernel
On 06/17/2016 04:01 AM, Gabriele Paoloni wrote:
> Hi Lorenzo and All
>
>> -----Original Message-----
>> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
>> Sent: 16 June 2016 18:49
>> To: Christopher Covington
>> Cc: Tomasz Nowicki; Duc Dang; liudongdong (C); Sinan Kaya; Jeff Hugo;
>> Gabriele Paoloni; Jon Masters; Mark Salter; Suravee Suthikulpanit;
>> Jayachandran C; David Daney; Robert Richter; Hanjun Guo; linux-arm-
>> kernel at lists.infradead.org; Catalin Marinas; Will Deacon; Bjorn Helgaas;
>> Ganapatrao Kulkarni; linux-kernel at vger.kernel.org
>> Subject: Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling
>> for ACPI based PCI host controller
>>
>> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>>> From: Tomasz Nowicki <tn@semihalf.com>
>>>
>>> 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 <tn@semihalf.com>
>>> ---
>>> 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 ?
> From my perspective I think the easiest solution is to keep this
> quirk mechanism in place and then review vendor by vendor solution
> as they are pushed to the mailing list; if some vendors are abusing
> of some addresses/resources then they can be rejected...
Using pci_ecam_create() works well for QDF2432:
https://us.codeaurora.org/cgit/quic/server/kernel/commit/?h=cov/4.7-rc3-testing&id=ca7439a54ddd8612b435c797ffc54f4e19f03e2b
Cheers,
Cov
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2016-06-21 9:33 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-17 21:37 [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller Duc Dang
2016-06-20 9:42 ` Lorenzo Pieralisi
2016-06-20 9:42 ` Lorenzo Pieralisi
2016-06-20 17:17 ` Christopher Covington
2016-06-20 17:17 ` Christopher Covington
2016-06-20 19:12 ` Duc Dang
2016-06-20 19:12 ` Duc Dang
2016-06-21 8:42 ` Duc Dang
2016-06-21 8:42 ` Duc Dang
2016-06-21 9:26 ` Lorenzo Pieralisi
2016-06-21 9:26 ` Lorenzo Pieralisi
2016-06-21 9:29 ` Duc Dang
2016-06-21 9:29 ` Duc Dang
-- strict thread matches above, loose matches on Subject: below --
2016-06-15 15:34 [RFC PATCH v3 1/2] ACPI/PCI: Check platform specific ECAM quirks Christopher Covington
2016-06-15 15:34 ` [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller Christopher Covington
2016-06-15 15:34 ` Christopher Covington
2016-06-16 17:48 ` Lorenzo Pieralisi
2016-06-16 17:48 ` Lorenzo Pieralisi
2016-06-17 8:01 ` Gabriele Paoloni
2016-06-17 8:01 ` Gabriele Paoloni
2016-06-17 14:13 ` Christopher Covington
2016-06-17 14:13 ` Christopher Covington
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.