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

Tomasz Nowicki
2015-11-06T12:40:46Z

On 06.11.2015 12:46, Jiang Liu wrote:
> On 2015/11/6 18:37, Tomasz Nowicki wrote:
>> On 06.11.2015 09:52, Jiang Liu wrote:
>>> 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 ?
>>> Hi Lorenzo and Tomasz,
>>> With a cup of coffee, I got myself awake eventually:)
>>> Now we are going to talk about IO port on IA64, really a little
>>> complex:( Actually there are two types of translation.
>>> 1) A PCI domain has a 24-bit IO port address space, there may
>>> be multiple IO port address spaces in systems with multiple PCI
>>> domains. So the first type of translation is to translate domain
>>> specific IO port address into system global IO port address
>>> (iomem_resource) by
>>> res->start = acpi_des->start + acpi_des->translation_offset
>>>
>>> 2) IA64 needs to map IO port address spaces into MMIO address
>>> space because it has no instructions to access IO ports directly.
>>> So IA64 has reserved a MMIO range to map IO port address spaces.
>>> This type of translation relies on architecture specific information
>>> instead of ACPI descriptors.
>>>
>>> On the other hand, ACPI specification has defined "I/O to Memory
>>> Translation" flag and "Memory to I/O Translation" flag in
>>> ACPI Extended Address Space Descriptor,
>>
>> Based on 2) and above, does it mean IA64 should use "ACPI Extended
>> Address Space Descriptor" for its PCI bridge IO windows only?
>>
>> but current implementation
>>> doesn't really support such a use case. So we need to find a way
>>> out here. Could you please help to provide more information about
>>> PCI host bridge resource descriptor implementation details on
>>> ARM64?
>>>
>>
>> Sure, ARM64 (0-16M IO space) QEMU example:
>> DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
>> 0x00000000, // Granularity
>> 0x00000000, // Range Minimum
>> 0x0000FFFF, // Range Maximum
>> 0x3EFF0000, // Translation Offset
>> 0x00010000, // Length
>> ,, , TypeStatic)
> The above DWordIO resource descriptor doesn't confirm to the ACPI spec.
> According to my understanding, ARM/ARM64 has no concept of IO port
> address space, so the PCI host bridge will map IO port on PCI side
> onto MMIO on host side. In other words, PCI host bridge on ARM64
> implement a IO Port->MMIO translation instead of a IO Port->IO Port
> translation. If that's true, it should use 'TypeTranslation' instead
> of 'TypeStatic'. And kernel ACPI resource parsing interface doesn't
> support 'TypeTranslation' yet, so we need to find a solution for it.

I think you are right, we need TypeTranslation flag for ARM64 DWordIO
descriptors and an extra kernel patch to support it.

Thanks,
Tomasz