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

[Jiang Liu <jiang.liu@linux.intel.com>]

On 2015/11/6 2:19, Lorenzo Pieralisi wrote:
> 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 ?
Yes, you are right, but seems I have done something wrong here.
Currently res->start = acpi_des->start + acpi_des->translation_offset,
which seems wrong. I will try to check IA64 side again and find
a fix for this.