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

[Tomasz Nowicki]

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
--
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/