Re: [RFC PATCH 4/4] arm64, acpi, pci: Provide arch-specific calls for PCI host bridge dirver (PNP0A03).
From: Tomasz Nowicki
Date: Wed Nov 12 2014 - 03:47:41 EST
W dniu 07.11.2014 o 15:55, Liviu Dudau pisze:
On Fri, Nov 07, 2014 at 01:27:56PM +0000, Tomasz Nowicki wrote:
Code inspired by arch/x86/pci/acpi.c and arch/ia64/pci/pci.c files.
The main reasons why we need this:
- parse and manage resources (BUS, IO and MEM)
- create pci root bus and enumerate its children
- provide PCI config space accessors (via MMCONFIG)
Hi Tomasz,
I have some comments to your code:
Signed-off-by: Tomasz Nowicki <tomasz.nowicki@xxxxxxxxxx>
---
arch/arm64/include/asm/pci.h | 8 +
arch/arm64/kernel/pci.c | 401 +++++++++++++++++++++++++++++++++++++++++--
2 files changed, 391 insertions(+), 18 deletions(-)
diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
index fded096..ecd940f 100644
--- a/arch/arm64/include/asm/pci.h
+++ b/arch/arm64/include/asm/pci.h
@@ -33,6 +33,14 @@ static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
extern int isa_dma_bridge_buggy;
#ifdef CONFIG_PCI
+struct pci_controller {
+ struct acpi_device *companion;
+ int segment;
+ int node; /* nearest node with memory or NUMA_NO_NODE for global allocation */
+};
+
+#define PCI_CONTROLLER(busdev) ((struct pci_controller *) busdev->sysdata)
I am trying to move away from the model where the architecture dictates the shape
of the pci_controller structure. Can I suggest that you put all this code and the
one under #ifdef CONFIG_ACPI in a separate file, say arch/arm64/kernel/pci-acpi.c ?
Or that you add an #ifdef CONFIG_ACPI around this structure definition?
I can't see anyone using this definition outside ACPI (due to struct acpi_device
dependency) and I would like to avoid it conflicting with other host bridge
drivers trying to define the same name structure.
That is good point, will do, thanks.
+
static inline int pci_proc_domain(struct pci_bus *bus)
{
return 1;
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index 42fb195..cc34ded 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -1,6 +1,10 @@
/*
- * Code borrowed from powerpc/kernel/pci-common.c
+ * Code borrowed from powerpc/kernel/pci-common.c and arch/ia64/pci/pci.c
*
+ * Copyright (c) 2002, 2005 Hewlett-Packard Development Company, L.P.
+ * David Mosberger-Tang <davidm@xxxxxxxxxx>
+ * Bjorn Helgaas <bjorn.helgaas@xxxxxx>
+ * Copyright (C) 2004 Silicon Graphics, Inc.
* Copyright (C) 2003 Anton Blanchard <anton@xxxxxxxxxx>, IBM
* Copyright (C) 2014 ARM Ltd.
*
@@ -17,10 +21,16 @@
#include <linux/mm.h>
#include <linux/of_pci.h>
#include <linux/of_platform.h>
+#include <linux/of_address.h>
#include <linux/slab.h>
+#include <linux/pci.h>
+#include <linux/acpi.h>
+#include <linux/mmconfig.h>
#include <asm/pci-bridge.h>
+#define PREFIX "PCI: "
Where is this used?
Nowhere here, will remove/improve.
+
/*
* Called after each bus is probed, but before its children are examined
*/
@@ -43,7 +53,8 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
*/
int pcibios_add_device(struct pci_dev *dev)
{
- dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+ if (acpi_disabled)
+ dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
return 0;
}
@@ -54,45 +65,399 @@ static bool dt_domain_found = false;
void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
{
- int domain = of_get_pci_domain_nr(parent->of_node);
+ int domain = -1;
- if (domain >= 0) {
- dt_domain_found = true;
- } else if (dt_domain_found == true) {
- dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
- parent->of_node->full_name);
- return;
+ if (acpi_disabled) {
+ domain = of_get_pci_domain_nr(parent->of_node);
+
+ if (domain >= 0) {
+ dt_domain_found = true;
+ } else if (dt_domain_found == true) {
+ dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
+ parent->of_node->full_name);
+ return;
+ } else {
+ domain = pci_get_new_domain_nr();
+ }
} else {
- domain = pci_get_new_domain_nr();
+ /*
+ * Take the domain nr from associated private data.
+ * Case where bus has parent is covered in pci_alloc_bus()
+ */
+ if (!parent)
+ domain = PCI_CONTROLLER(bus)->segment;
I would also like to wrap this into an ACPI specific function. Reason for it
is that when I get to split the pci_host_bridge creation out of pci_create_root_bus()
this will become a pci_controller property.
Make sense for me.
}
bus->domain_nr = domain;
}
#endif
+static char __iomem *pci_dev_base(unsigned int seg, unsigned int bus,
+ unsigned int devfn)
+{
+ struct pci_mmcfg_region *cfg = pci_mmconfig_lookup(seg, bus);
+
+ if (cfg && cfg->virt)
+ return cfg->virt + (PCI_MMCFG_BUS_OFFSET(bus) | (devfn << 12));
+ return NULL;
+}
+
/*
* raw_pci_read/write - Platform-specific PCI config space access.
- *
- * Default empty implementation. Replace with an architecture-specific setup
- * routine, if necessary.
*/
int raw_pci_read(unsigned int domain, unsigned int bus,
unsigned int devfn, int reg, int len, u32 *val)
{
- return -EINVAL;
+ char __iomem *addr;
+
+ if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) {
+err: *val = -1;
I believe the general usage is to have labels on their own line.
+ return -EINVAL;
+ }
+
+ rcu_read_lock();
+ addr = pci_dev_base(domain, bus, devfn);
+ if (!addr) {
+ rcu_read_unlock();
+ goto err;
+ }
+
+ switch (len) {
+ case 1:
+ *val = readb(addr + reg);
+ break;
+ case 2:
+ *val = readw(addr + reg);
+ break;
+ case 4:
+ *val = readl(addr + reg);
+ break;
+ }
+ rcu_read_unlock();
+
+ return 0;
}
int raw_pci_write(unsigned int domain, unsigned int bus,
unsigned int devfn, int reg, int len, u32 val)
{
- return -EINVAL;
+ char __iomem *addr;
+
+ if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095)))
+ return -EINVAL;
+
+ rcu_read_lock();
+ addr = pci_dev_base(domain, bus, devfn);
+ if (!addr) {
+ rcu_read_unlock();
+ return -EINVAL;
+ }
+
+ switch (len) {
+ case 1:
+ writeb(val, addr + reg);
+ break;
+ case 2:
+ writew(val, addr + reg);
+ break;
+ case 4:
+ writel(val, addr + reg);
+ break;
+ }
+ rcu_read_unlock();
+
+ return 0;
}
These raw_pci_{read,write} functions are all ACPI specific so they can move
into the new file as well.
Got it!
#ifdef CONFIG_ACPI
+static int pci_read(struct pci_bus *bus, unsigned int devfn, int where,
+ int size, u32 *value)
+{
+ return raw_pci_read(pci_domain_nr(bus), bus->number,
+ devfn, where, size, value);
+}
+
+static int pci_write(struct pci_bus *bus, unsigned int devfn, int where,
+ int size, u32 value)
+{
+ return raw_pci_write(pci_domain_nr(bus), bus->number,
+ devfn, where, size, value);
+}
+
+struct pci_ops pci_root_ops = {
+ .read = pci_read,
+ .write = pci_write,
+};
+
+static struct pci_controller *alloc_pci_controller(int seg)
+{
+ struct pci_controller *controller;
+
+ controller = kzalloc(sizeof(*controller), GFP_KERNEL);
+ if (!controller)
+ return NULL;
+
+ controller->segment = seg;
+ return controller;
+}
+
+struct pci_root_info {
+ struct acpi_device *bridge;
+ struct pci_controller *controller;
+ struct list_head resources;
+ struct resource *res;
+ resource_size_t *res_offset;
Why do you need to keep an array of res_offsets here? You only seem
to be using the values once when you add the resource to the host
bridge windows.
This is what I noticed too but did not improve that at the end. Let me
fix that in the next ver.
+ unsigned int res_num;
+ char *name;
+};
+
+static acpi_status resource_to_window(struct acpi_resource *resource,
+ struct acpi_resource_address64 *addr)
+{
+ acpi_status status;
+
+ /*
+ * We're only interested in _CRS descriptors that are
+ * - address space descriptors for memory
+ * - non-zero size
+ * - producers, i.e., the address space is routed downstream,
+ * not consumed by the bridge itself
+ */
+ status = acpi_resource_to_address64(resource, addr);
+ if (ACPI_SUCCESS(status) &&
+ (addr->resource_type == ACPI_MEMORY_RANGE ||
+ addr->resource_type == ACPI_IO_RANGE) &&
+ addr->address_length &&
+ addr->producer_consumer == ACPI_PRODUCER)
+ return AE_OK;
+
+ return AE_ERROR;
+}
+
+static acpi_status count_window(struct acpi_resource *resource, void *data)
+{
+ unsigned int *windows = (unsigned int *) data;
+ struct acpi_resource_address64 addr;
+ acpi_status status;
+
+ status = resource_to_window(resource, &addr);
+ if (ACPI_SUCCESS(status))
+ (*windows)++;
+
+ return AE_OK;
+}
+
+static acpi_status add_window(struct acpi_resource *res, void *data)
+{
+ struct pci_root_info *info = data;
+ struct resource *resource;
+ struct acpi_resource_address64 addr;
+ acpi_status status;
+ unsigned long flags;
+ struct resource *root;
+ u64 start;
+
+ /* Return AE_OK for non-window resources to keep scanning for more */
+ status = resource_to_window(res, &addr);
+ if (!ACPI_SUCCESS(status))
+ return AE_OK;
+
+ if (addr.resource_type == ACPI_MEMORY_RANGE) {
+ flags = IORESOURCE_MEM;
+ root = &iomem_resource;
+ } else if (addr.resource_type == ACPI_IO_RANGE) {
+ flags = IORESOURCE_IO;
+ root = &ioport_resource;
+ } else
+ return AE_OK;
+
+ start = addr.minimum + addr.translation_offset;
+
+ resource = &info->res[info->res_num];
+ resource->name = info->name;
+ resource->flags = flags;
+ resource->start = start;
+ resource->end = resource->start + addr.address_length - 1;
+
+ if (flags & IORESOURCE_IO) {
+ unsigned long port;
+ int err;
+
+ err = pci_register_io_range(start, addr.address_length);
+ if (err)
+ return AE_OK;
+
+ port = pci_address_to_pio(start);
+ if (port == (unsigned long)-1)
+ return AE_OK;
+
+ resource->start = port;
+ resource->end = port + addr.address_length - 1;
+
+ if (pci_remap_iospace(resource, start) < 0)
+ return AE_OK;
You seem to leave around invalid resources every time you return in this code
path due to your choice of ignoring error conditions.
I think there are a few things that can be streamlined in this patch, but
overall I think it looks promising.
Thanks Liviu!
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/