Re: [Patch v7 4/7] PCI/ACPI: Add interface acpi_pci_root_create()

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Thu, Nov 05, 2015 at 03:21:34PM +0100, Tomasz Nowicki wrote:
> On 14.10.2015 08:29, Jiang Liu wrote:

[...]

> >+static void acpi_pci_root_validate_resources(struct device *dev,
> >+					     struct list_head *resources,
> >+					     unsigned long type)
> >+{
> >+	LIST_HEAD(list);
> >+	struct resource *res1, *res2, *root = NULL;
> >+	struct resource_entry *tmp, *entry, *entry2;
> >+
> >+	BUG_ON((type & (IORESOURCE_MEM | IORESOURCE_IO)) == 0);
> >+	root = (type & IORESOURCE_MEM) ? &iomem_resource : &ioport_resource;
> >+
> >+	list_splice_init(resources, &list);
> >+	resource_list_for_each_entry_safe(entry, tmp, &list) {
> >+		bool free = false;
> >+		resource_size_t end;
> >+
> >+		res1 = entry->res;
> >+		if (!(res1->flags & type))
> >+			goto next;
> >+
> >+		/* Exclude non-addressable range or non-addressable portion */
> >+		end = min(res1->end, root->end);
> >+		if (end <= res1->start) {
> >+			dev_info(dev, "host bridge window %pR (ignored, not CPU addressable)\n",
> >+				 res1);
> >+			free = true;
> >+			goto next;
> >+		} else if (res1->end != end) {
> >+			dev_info(dev, "host bridge window %pR ([%#llx-%#llx] ignored, not CPU addressable)\n",
> >+				 res1, (unsigned long long)end + 1,
> >+				 (unsigned long long)res1->end);
> >+			res1->end = end;
> >+		}
> >+
> >+		resource_list_for_each_entry(entry2, resources) {
> >+			res2 = entry2->res;
> >+			if (!(res2->flags & type))
> >+				continue;
> >+
> >+			/*
> >+			 * I don't like throwing away windows because then
> >+			 * our resources no longer match the ACPI _CRS, but
> >+			 * the kernel resource tree doesn't allow overlaps.
> >+			 */
> >+			if (resource_overlaps(res1, res2)) {
> >+				res2->start = min(res1->start, res2->start);
> >+				res2->end = max(res1->end, res2->end);
> >+				dev_info(dev, "host bridge window expanded to %pR; %pR ignored\n",
> >+					 res2, res1);
> >+				free = true;
> >+				goto next;
> >+			}
> >+		}
> >+
> >+next:
> >+		resource_list_del(entry);
> >+		if (free)
> >+			resource_list_free_entry(entry);
> >+		else
> >+			resource_list_add_tail(entry, resources);
> >+	}
> >+}
> >+
> >+int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
> >+{
> >+	int ret;
> >+	struct list_head *list = &info->resources;
> >+	struct acpi_device *device = info->bridge;
> >+	struct resource_entry *entry, *tmp;
> >+	unsigned long flags;
> >+
> >+	flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_MEM_8AND16BIT;
> >+	ret = acpi_dev_get_resources(device, list,
> >+				     acpi_dev_filter_resource_type_cb,
> >+				     (void *)flags);
> >+	if (ret < 0)
> >+		dev_warn(&device->dev,
> >+			 "failed to parse _CRS method, error code %d\n", ret);
> >+	else if (ret == 0)
> >+		dev_dbg(&device->dev,
> >+			"no IO and memory resources present in _CRS\n");
> >+	else {
> >+		resource_list_for_each_entry_safe(entry, tmp, list) {
> >+			if (entry->res->flags & IORESOURCE_DISABLED)
> >+				resource_list_destroy_entry(entry);
> >+			else
> >+				entry->res->name = info->name;
> >+		}
> >+		acpi_pci_root_validate_resources(&device->dev, list,
> >+						 IORESOURCE_MEM);
> >+		acpi_pci_root_validate_resources(&device->dev, list,
> >+						 IORESOURCE_IO);
> 
> It is not clear to me why we need these two calls above ^^^. We are
> using pci_acpi_root_add_resources(info) later. Is it not enough?
> 
> Also, I cannot use acpi_pci_probe_root_resources() in my ARM64 PCI
> driver. It is because acpi_dev_get_resources is adding
> translation_offset to IO ranges start address and then:
> acpi_pci_root_validate_resources(&device->dev, list,
> 				 IORESOURCE_IO);
> rejects that IO regions as it is out of my 0x0-SZ_16M window.
> 
> Does acpi_pci_probe_root_resources meant to be x86 specific and I
> should avoid using it?

IIUC, you _have_ to have the proper translation_offset to map the bridge
window into the IO address space:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/348708.html

Then, using the offset, you should do something ia64 does, namely,
retrieve the CPU address corresponding to IO space (see arch/ia64/pci/pci.c
- add_io_space()) and map it in the physical address space by using
pci_remap_iospace(), it is similar to what we have to do with DT.

It is extremely confusing and I am not sure I got it right myself,
I am still grokking ia64 code to understand what it really does.

So basically, the IO bridge window coming from acpi_dev_get_resource()
should represent the IO space in 0 - 16M, IIUC.

By using the offset (that was initialized using translation_offset) and
the resource->start, you can retrieve the cpu address that you need to
actually map the IO space, since that's what we do on ARM (ie the
IO resource is an offset into the virtual address space set aside
for IO).

Confusing, to say the least. Jiang, did I get it right ?

Lorenzo