On Wed, Sep 10, 2014 at 04:10:26PM +0100, Lorenzo Pieralisi wrote:

On Wed, Sep 10, 2014 at 03:22:41PM +0100, Liviu Dudau wrote:

quoted

On Tue, Sep 09, 2014 at 02:35:46PM +0100, Lorenzo Pieralisi wrote:

quoted

On Mon, Sep 08, 2014 at 02:54:30PM +0100, Liviu Dudau wrote:

quoted

Provide a function to parse the PCI DT ranges that can be used to
create a pci_host_bridge structure together with its associated
bus.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Grant Likely <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 drivers/of/of_pci.c    | 102 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_pci.h |  11 ++++++
 2 files changed, 113 insertions(+)

diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index a107edb..36701da 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c

@@ -1,7 +1,9 @@
 #include <linux/kernel.h>
 #include <linux/export.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_pci.h>
+#include <linux/slab.h>
 
 static inline int __of_pci_pci_compare(struct device_node *node,
 				       unsigned int data)

@@ -123,6 +125,106 @@ int of_pci_get_domain_nr(struct device_node *node, bool allocate_if_missing)
 }
 EXPORT_SYMBOL_GPL(of_pci_get_domain_nr);
 
+/**
+ * of_pci_get_host_bridge_resources - Parse PCI host bridge resources from DT
+ * @dev: device node of the host bridge having the range property
+ * @busno: bus number associated with the bridge root bus
+ * @bus_max: maximum number of busses for this bridge
+ * @resources: list where the range of resources will be added after DT parsing
+ * @io_base: pointer to a variable that will contain on return the physical
+ * address for the start of the I/O range.
+ *
+ * It is the callers job to free the @resources list.
+ *
+ * This function will parse the "ranges" property of a PCI host bridge device
+ * node and setup the resource mapping based on its content. It is expected
+ * that the property conforms with the Power ePAPR document.

You should also define what it returns and when.

Thanks, will do.

quoted

quoted

+ *
+ */
+int of_pci_get_host_bridge_resources(struct device_node *dev,
+			unsigned char busno, unsigned char bus_max,
+			struct list_head *resources, resource_size_t *io_base)
+{
+	struct resource *res;
+	struct resource *bus_range;
+	struct of_pci_range range;
+	struct of_pci_range_parser parser;
+	char range_type[4];
+	int err;
+
+	bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
+	if (!bus_range)
+		return -ENOMEM;
+
+	pr_info("PCI host bridge %s ranges:\n", dev->full_name);
+
+	err = of_pci_parse_bus_range(dev, bus_range);
+	if (err) {
+		bus_range->start = busno;
+		bus_range->end = bus_max;
+		bus_range->flags = IORESOURCE_BUS;
+		pr_info("  No bus range found for %s, using %pR\n",
+			dev->full_name, &bus_range);
+	} else {
+		if (bus_range->end > bus_range->start + bus_max)
+			bus_range->end = bus_range->start + bus_max;
+	}
+	pci_add_resource(resources, bus_range);

This means that eg in the PCI generic host controller I cannot "filter"
the bus resource, unless I remove it, "filter" it, and add it again.

I'm not sure what you mean. Why do you have to remove the bus resource and
add it again? What you get back from of_pci_get_host_bridge_resources() is
a list of resources as they have been parsed from DT. You now have the option
of doing any filtering that you might have done pre-v10 in the
pcibios_fixup_bridge_ranges() but that is only on the list that was returned
from of_pci_get_host_bridge_resources(). At this moment no root bus or
host bridge structure has been created so no resource was added to those.
With the filtered list you can use it to call pci_scan_root_bus() and *then*
it gets added to the pci_host_bridge structure.

You are right sorry. As discussed, I just have to remove the resources
assignment in the respective host controller drivers (because you do
that in the API) and grab the resource pointers to "filter" them.

quoted

quoted

I certainly can't filter a resource that has been already added without
removing it first.

Thoughts ?

Hope I have explained what happens. Please let me know if you have any other
comments.

There were other comments below ;)

My PgDn key is broken ;)

Thanks,
Lorenzo

quoted

Best regards,
Liviu

quoted

quoted

+
+	/* Check for ranges property */
+	err = of_pci_range_parser_init(&parser, dev);
+	if (err)
+		goto parse_failed;
+
+	pr_debug("Parsing ranges property...\n");
+	for_each_of_pci_range(&parser, &range) {
+		/* Read next ranges element */
+		if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_IO)
+			snprintf(range_type, 4, " IO");
+		else if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_MEM)
+			snprintf(range_type, 4, "MEM");
+		else
+			snprintf(range_type, 4, "err");
+		pr_info("  %s %#010llx..%#010llx -> %#010llx\n", range_type,
+			range.cpu_addr, range.cpu_addr + range.size - 1,
+			range.pci_addr);
+
+		/*
+		 * If we failed translation or got a zero-sized region
+		 * then skip this range
+		 */
+		if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
+			continue;
+
+		res = kzalloc(sizeof(struct resource), GFP_KERNEL);
+		if (!res) {
+			err = -ENOMEM;
+			goto parse_failed;
+		}
+
+		err = of_pci_range_to_resource(&range, dev, res);
+		if (err) {
+			kfree(res);

You might want to add a label to free res to make things more uniform.

Sorry, not following you. How would a label help here?

quoted

quoted

quoted

+			goto parse_failed;
+		}
+
+		if (resource_type(res) == IORESOURCE_IO) {
+			if (*io_base)

You do not zero io_base in the first place so you should ask the API
user to do that. Is 0 a valid value BTW ? If it is you've got to resort
to something else to detect multiple IO resources.

No, zero is not a valid value. It is the cpu_addr value from the IO range, I'm
hopying that no one is crazy enough to map PCI address space at CPU address zero.
Thanks for spotting the lack of initialisation though, I need to fix it.

Best regards,
Liviu

quoted

quoted

Lorenzo

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?