On 14.10.2015 08:29, Jiang Liu wrote:
> Introduce common interface acpi_pci_root_create() and related data
> structures to create PCI root bus for ACPI PCI host bridges. It will
> be used to kill duplicated arch specific code for IA64 and x86. It may
> also help ARM64 in future.
>
> Reviewed-by: Lorenzo Pieralisi <[email protected]>
> Tested-by: Tony Luck <[email protected]>
> Signed-off-by: Jiang Liu <[email protected]>
> Signed-off-by: Liu Jiang <[email protected]>
> ---
> drivers/acpi/pci_root.c | 204 ++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/pci-acpi.h | 24 ++++++
> 2 files changed, 228 insertions(+)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 393706a5261b..850d7bf0c873 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -652,6 +652,210 @@ static void acpi_pci_root_remove(struct acpi_device *device)
> kfree(root);
> }
>
> +/*
> + * Following code to support acpi_pci_root_create() is copied from
> + * arch/x86/pci/acpi.c and modified so it could be reused by x86, IA64
> + * and ARM64.
> + */
> +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?
Thanks,
Tomasz
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
On 2015/11/5 22:21, Tomasz Nowicki wrote:
> On 14.10.2015 08:29, Jiang Liu wrote:
>> Introduce common interface acpi_pci_root_create() and related data
>> structures to create PCI root bus for ACPI PCI host bridges. It will
>> be used to kill duplicated arch specific code for IA64 and x86. It may
>> also help ARM64 in future.
>>
>> Reviewed-by: Lorenzo Pieralisi <[email protected]>
>> Tested-by: Tony Luck <[email protected]>
>> Signed-off-by: Jiang Liu <[email protected]>
>> Signed-off-by: Liu Jiang <[email protected]>
>> ---
>> drivers/acpi/pci_root.c | 204
>> ++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/pci-acpi.h | 24 ++++++
>> 2 files changed, 228 insertions(+)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index 393706a5261b..850d7bf0c873 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -652,6 +652,210 @@ static void acpi_pci_root_remove(struct
>> acpi_device *device)
>> kfree(root);
>> }
>>
>> +/*
>> + * Following code to support acpi_pci_root_create() is copied from
>> + * arch/x86/pci/acpi.c and modified so it could be reused by x86, IA64
>> + * and ARM64.
>> + */
>> +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?
Hi Tomasz,
acpi_pci_root_validate_resources() will try adjust (or fix)
conflicting resources among all resources of the PCI host bridge,
but pci_acpi_root_add_resources() only rejects conflicting resources.
>
> 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?
It should be generic, but we have some issue in support of
translation_offset. I'm trying to get this fixed.
Thanks,
Gerry
>
> Thanks,
> Tomasz
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.
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, 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?
>
> Lorenzo
>
On 05.11.2015 19: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.
Yes, you are right IMO.
>
> 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).
>
Right, that is clear to me, below my current implementation example
which works for ARM64 now:
static struct acpi_pci_root_ops acpi_pci_root_ops = {
[...]
.prepare_resources = pci_acpi_root_prepare_resources,
};
static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
{
struct list_head *list = &ci->resources;
struct acpi_device *device = ci->bridge;
struct resource_entry *entry, *tmp;
unsigned long flags;
int ret;
flags = IORESOURCE_IO | IORESOURCE_MEM;
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);
return ret;
} else if (ret == 0)
dev_dbg(&device->dev,
"no IO and memory resources present in _CRS\n");
resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
struct resource *res = entry->res;
if (entry->res->flags & IORESOURCE_DISABLED)
resource_list_destroy_entry(entry);
else
res->name = ci->name;
/*
* TODO: need to move pci_register_io_range() function out
* of drivers/of/address.c for both used by DT and ACPI
*/
if (res->flags & IORESOURCE_IO) {
unsigned long port;
int err;
resource_size_t length = res->end - res->start;
resource_size_t start = res->start;
err = pci_register_io_range(res->start, length);
if (err) {
resource_list_destroy_entry(entry);
continue;
}
port = pci_address_to_pio(res->start);
if (port == (unsigned long)-1) {
resource_list_destroy_entry(entry);
continue;
}
res->start = port;
res->end = res->start + length;
entry->offset = port - (start - entry->offset);
if (pci_remap_iospace(res, start) < 0)
resource_list_destroy_entry(entry);
}
}
return ret;
}
As you see acpi_dev_get_resources returns:
res->start = acpi_des->start + acpi_des->translation_offset (CPU address)
which then must be adjusted as you described to get io port and call
pci_remap_iospace.
This is also why I can not use acpi_pci_probe_root_resources here. Lets
assume we have IO range like that DSDT table form QEMU:
DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
0x00000000, // Granularity
0x00000000, // Range Minimum
0x0000FFFF, // Range Maximum
0x3EFF0000, // Translation Offset
0x00010000, // Length
,, , TypeStatic)
so see acpi_dev_get_resources returns res->start = acpi_des->start (0x0)
+ acpi_des->translation_offset(0x3EFF0000) = 0x3EFF0000. This will be
rejected in acpi_pci_root_validate_resources() as 0x3EFF0000 does not
fit within 0-16M.
My question is if acpi_pci_probe_root_resources is handling
translation_offset properly and if we have some silent assumption
specific for e.g. ia64 here.
Thanks for help in looking at it.
Tomasz
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)
You can also have a look at my implementation example in mail to Lorenzo.
Thanks,
Tomasz
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.
Thanks,
Gerry
>
> You can also have a look at my implementation example in mail to Lorenzo.
>
> Thanks,
> Tomasz
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
On Fri, Nov 06, 2015 at 04:52:47PM +0800, Jiang Liu wrote:
[...]
> >>> +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
And that's what I do not understand, or better I do not understand
why the acpi_pci_root_validate_resources (for IO) does not fail on ia64,
since that should work as arm64, namely IO ports are mapped through
MMIO.
I think the check in acpi_pci_root_validate_resources() (for
IORESOURCE_IO) fails at present on ia64 too, correct ?
If not, how can it work ? res->start definitely contains the
CPU physical address mapping IO space after adding the
translation_offset, so the check in acpi_pci_root_validate_resources()
for IO can't succeed.
In add_io_space() ia64 does the same thing as Tomasz has to do,
namely overwriting the res->start and end with the offset into
the virtual address space allocated for IO, which is different from
the CPU physical address allocated to IO space.
Please correct me if I am wrong.
> 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.
That's how ARM64 works too, the IO space resources are an offset into
a chunk of virtual address space allocated to PCI IO memory, so it
seems to me that arm64 and ia64 should work the same way, and that
at present acpi_pci_root_validate_resources() should fail on ia64 too.
Thanks,
Lorenzo
>
> 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, 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?
>
> >
> > Lorenzo
> >
>
On 2015/11/6 20:40, Tomasz Nowicki wrote:
> 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:
>>> 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.
How about the attached to patch to support TypeTranslation?
It only passes compilation:)
>
> Thanks,
> Tomasz
On Fri, Nov 06, 2015 at 09:22:46PM +0800, Jiang Liu wrote:
> On 2015/11/6 20:40, Tomasz Nowicki wrote:
> > 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:
> >>> 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.
> How about the attached to patch to support TypeTranslation?
> It only passes compilation:)
Eh, hopefully there are not any ACPI tables out there with that bit
set that work _today_ and would not work with the patch attached :)
My question is still there: do we want to handle the same problem
as ia64 has in a different manner ? Certainly we won't be able
to update ia64 platforms ACPI tables, so we would end up with
two platforms handling IO resources in different ways unless I am
missing something here.
BTW, why would we add offset to res->start only if TypeTranslation is
clear ? Is not that something we would do just to make things "work" ?
That flag has no bearing on the offset, only on the resource type AFAIK.
This without taking into account ARM64 systems shipping with ACPI
tables that does not set the TypeTranslation at present.
On top of that, I noticed that core ACPI code handles Sparse
Translation (ie _TRS), that should be considered meaningful only if _TTP
is set (and that's not checked).
Thoughts ?
Thanks,
Lorenzo
On 2015/11/6 22:45, Lorenzo Pieralisi wrote:
> On Fri, Nov 06, 2015 at 09:22:46PM +0800, Jiang Liu wrote:
>> On 2015/11/6 20:40, Tomasz Nowicki wrote:
>>> 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:
>>>>> 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.
>> How about the attached to patch to support TypeTranslation?
>> It only passes compilation:)
>
> Eh, hopefully there are not any ACPI tables out there with that bit
> set that work _today_ and would not work with the patch attached :)
>
> My question is still there: do we want to handle the same problem
> as ia64 has in a different manner ? Certainly we won't be able
> to update ia64 platforms ACPI tables, so we would end up with
> two platforms handling IO resources in different ways unless I am
> missing something here.
There are some difference between IA64 and ARM64.
On IA64, it supports 16M IO address space per PCI domain and 256 PCI
domains at max. So the system IO address space is 16M * 256 = 4G.
So it does two level translations to support IO port
1) translate PCI bus local IO port address into system global IO port
address by adding acpi_des->translation_offset.
2) translate the 4G system IO port address space into MMIO address.
IA64 has reserved a 4G space for IO port mapping. This translation
is done by arch specific method.
In other word, IA64 needs two level translation, but ACPI only provides
on (trans_type, trans_offset) pair for encoding, so it's used for step 1).
For ARM64, I think currently it only needs step 2).
>
> BTW, why would we add offset to res->start only if TypeTranslation is
> clear ? Is not that something we would do just to make things "work" ?
> That flag has no bearing on the offset, only on the resource type AFAIK.
It's not a hack, but a way to interpret ACPI spec:)
With current linux resource management framework, we need to allocate
both MMIO and IO port address space range for an ACPI resource of type
'TypeTranslation'. And struct resource could be either IO port or MMIO,
not both. So the choice is to keep the resource as IO port, and let
arch code to build the special MMIO mapping for it. Otherwise it will
break too many things if we convert the resource as MMIO.
That said, we need to add translation_offset to convert bus local
IO port address into system global IO port address if it's type of
TypeStatic, because ioresource_ioport uses system global IO port
address.
For an ACPI resource of type TypeTranslation, system global IO port
address equals bus local IO port address, and the translation_offset
is used to translate IO port address into MMIO address, so we shouldn't
add translation_offset to the IO port resource descriptor.
Thanks,
Gerry
>
> This without taking into account ARM64 systems shipping with ACPI
> tables that does not set the TypeTranslation at present.
>
> On top of that, I noticed that core ACPI code handles Sparse
> Translation (ie _TRS), that should be considered meaningful only if _TTP
> is set (and that's not checked).
Yes, that's a flaw:(
>
> Thoughts ?
>
> Thanks,
> Lorenzo
>
On 2015/11/6 23:32, Jiang Liu wrote:
> On 2015/11/6 22:45, Lorenzo Pieralisi wrote:
>> On Fri, Nov 06, 2015 at 09:22:46PM +0800, Jiang Liu wrote:
>>> On 2015/11/6 20:40, Tomasz Nowicki wrote:
>>>> 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:
>>>>>> 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.
>>> How about the attached to patch to support TypeTranslation?
>>> It only passes compilation:)
>>
>> Eh, hopefully there are not any ACPI tables out there with that bit
>> set that work _today_ and would not work with the patch attached :)
>>
>> My question is still there: do we want to handle the same problem
>> as ia64 has in a different manner ? Certainly we won't be able
>> to update ia64 platforms ACPI tables, so we would end up with
>> two platforms handling IO resources in different ways unless I am
>> missing something here.
> There are some difference between IA64 and ARM64.
> On IA64, it supports 16M IO address space per PCI domain and 256 PCI
> domains at max. So the system IO address space is 16M * 256 = 4G.
> So it does two level translations to support IO port
> 1) translate PCI bus local IO port address into system global IO port
> address by adding acpi_des->translation_offset.
> 2) translate the 4G system IO port address space into MMIO address.
> IA64 has reserved a 4G space for IO port mapping. This translation
> is done by arch specific method.
> In other word, IA64 needs two level translation, but ACPI only provides
> on (trans_type, trans_offset) pair for encoding, so it's used for step 1).
>
> For ARM64, I think currently it only needs step 2).
>
>>
>> BTW, why would we add offset to res->start only if TypeTranslation is
>> clear ? Is not that something we would do just to make things "work" ?
>> That flag has no bearing on the offset, only on the resource type AFAIK.
> It's not a hack, but a way to interpret ACPI spec:)
>
> With current linux resource management framework, we need to allocate
> both MMIO and IO port address space range for an ACPI resource of type
> 'TypeTranslation'. And struct resource could be either IO port or MMIO,
> not both. So the choice is to keep the resource as IO port, and let
> arch code to build the special MMIO mapping for it. Otherwise it will
> break too many things if we convert the resource as MMIO.
>
> That said, we need to add translation_offset to convert bus local
> IO port address into system global IO port address if it's type of
> TypeStatic, because ioresource_ioport uses system global IO port
> address.
>
> For an ACPI resource of type TypeTranslation, system global IO port
> address equals bus local IO port address, and the translation_offset
> is used to translate IO port address into MMIO address, so we shouldn't
> add translation_offset to the IO port resource descriptor.
One note for the TypeTranslation case, the arch code needs to reset
resource_win->offset to zero after setting up the MMIO map. Sample
code as below:
va = ioremap(resource_win->offset + res->start, resource_size(res));
resource_win->offset = 0;
Otherwise it will break pcibios_resource_to_bus() etc.
>
> Thanks,
> Gerry
>
>>
>> This without taking into account ARM64 systems shipping with ACPI
>> tables that does not set the TypeTranslation at present.
>>
>> On top of that, I noticed that core ACPI code handles Sparse
>> Translation (ie _TRS), that should be considered meaningful only if _TTP
>> is set (and that's not checked).
> Yes, that's a flaw:(
>
>>
>> Thoughts ?
>>
>> Thanks,
>> Lorenzo
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On 06.11.2015 14:22, Jiang Liu wrote:
> On 2015/11/6 20:40, Tomasz Nowicki wrote:
>> 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:
>>>> 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.
> How about the attached to patch to support TypeTranslation?
> It only passes compilation:)
Based on the further discussion, your draft patch looks good to me.
Lorenzo, do you agree?
Gerry, what would be the best way to approach with this, extra patch of
your set? or keep it separately, might be part of my set.
Thanks,
Tomasz
[CC'ing Arnd]
On Mon, Nov 09, 2015 at 03:07:38PM +0100, Tomasz Nowicki wrote:
> On 06.11.2015 14:22, Jiang Liu wrote:
> >On 2015/11/6 20:40, Tomasz Nowicki wrote:
> >>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:
> >>>>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.
> >How about the attached to patch to support TypeTranslation?
> >It only passes compilation:)
>
> Based on the further discussion, your draft patch looks good to me.
> Lorenzo, do you agree?
No, because I still do not understand the difference between ia64 and
arm64 (they both drive IO ports cycles through MMIO so the resource
descriptors content must be the same or better they must mean the same
thing). On top of that, this is something that was heavily debated for DT:
http://www.spinics.net/lists/arm-kernel/msg345633.html
and I would like to get Arnd and Bjorn opinion on this because we
should not "interpret" ACPI specifications, we should understand
what they are supposed to describe and write kernel code accordingly.
In particular, I would like to understand, for an eg DWordIO descriptor,
what Range Minimum, Range Maximum and Translation Offset represent,
they can't mean different things depending on the SW parsing them,
this totally defeats the purpose.
By the way, ia64 ioremaps the translation_offset (ie new_space()), so
basically that's the CPU physical address at which the PCI host bridge
map the IO space transactions), I do not think ia64 is any different from
arm64 in this respect, if it is please provide an HW description here from
the PCI bus perspective here (also an example of ia64 ACPI PCI host bridge
tables would help).
Thanks,
Lorenzo
On Monday 09 November 2015 17:10:43 Lorenzo Pieralisi wrote:
> On Mon, Nov 09, 2015 at 03:07:38PM +0100, Tomasz Nowicki wrote:
> > On 06.11.2015 14:22, Jiang Liu wrote:
> > >On 2015/11/6 20:40, Tomasz Nowicki wrote:
> > >>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:
> > >>>>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.
> > >How about the attached to patch to support TypeTranslation?
> > >It only passes compilation:)
> >
> > Based on the further discussion, your draft patch looks good to me.
> > Lorenzo, do you agree?
>
> No, because I still do not understand the difference between ia64 and
> arm64 (they both drive IO ports cycles through MMIO so the resource
> descriptors content must be the same or better they must mean the same
> thing). On top of that, this is something that was heavily debated for DT:
>
> http://www.spinics.net/lists/arm-kernel/msg345633.html
>
> and I would like to get Arnd and Bjorn opinion on this because we
> should not "interpret" ACPI specifications, we should understand
> what they are supposed to describe and write kernel code accordingly.
>
> In particular, I would like to understand, for an eg DWordIO descriptor,
> what Range Minimum, Range Maximum and Translation Offset represent,
> they can't mean different things depending on the SW parsing them,
> this totally defeats the purpose.
I have no clue about what those mean in ACPI though.
Generally speaking, each PCI domain is expected to have a (normally 64KB)
range of CPU addresses that gets translated into PCI I/O space the same
way that config space and memory space are handled.
This is true for almost every architecture except for x86, which uses
different CPU instructions for I/O space compared to the other spaces.
> By the way, ia64 ioremaps the translation_offset (ie new_space()), so
> basically that's the CPU physical address at which the PCI host bridge
> map the IO space transactions), I do not think ia64 is any different from
> arm64 in this respect, if it is please provide an HW description here from
> the PCI bus perspective here (also an example of ia64 ACPI PCI host bridge
> tables would help).
The main difference between ia64 and a lot of the other architectures (e.g.
sparc is different again) is that ia64 defines a logical address range
in terms of having a small number for each I/O space followed by the
offset within that space as a 'port number' and uses a mapping function
that is defined as
static inline void *__ia64_mk_io_addr (unsigned long port)
{
struct io_space *space = &io_space[IO_SPACE_NR(port)];
return (space->mmio_base | IO_SPACE_PORT(port););
}
static inline unsigned int inl(unsigned long port)
{
return *__ia64_mk_io_addr(port);
}
Most architectures allow only one I/O port range and put it at a fixed
virtual address so that inl() simply becomes
static inline u32 inl(unsigned long addr)
{
return readl(PCI_IOBASE + addr);
}
which noticeably reduces code size.
On some architectures (powerpc, arm, arm64), we then get the same simplified
definition with a fixed virtual address, and use pci_ioremap_io() or
something like that to to map a physical address range into this virtual
address window at the correct io_offset;
Arnd
On 2015/11/10 4:09, Arnd Bergmann wrote:
> On Monday 09 November 2015 17:10:43 Lorenzo Pieralisi wrote:
>> On Mon, Nov 09, 2015 at 03:07:38PM +0100, Tomasz Nowicki wrote:
>>> On 06.11.2015 14:22, Jiang Liu wrote:
>>>> On 2015/11/6 20:40, Tomasz Nowicki wrote:
>>>>> 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:
>>>>>>> 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.
>>>> How about the attached to patch to support TypeTranslation?
>>>> It only passes compilation:)
>>>
>>> Based on the further discussion, your draft patch looks good to me.
>>> Lorenzo, do you agree?
>>
>> No, because I still do not understand the difference between ia64 and
>> arm64 (they both drive IO ports cycles through MMIO so the resource
>> descriptors content must be the same or better they must mean the same
>> thing). On top of that, this is something that was heavily debated for DT:
>>
>> http://www.spinics.net/lists/arm-kernel/msg345633.html
>>
>> and I would like to get Arnd and Bjorn opinion on this because we
>> should not "interpret" ACPI specifications, we should understand
>> what they are supposed to describe and write kernel code accordingly.
>>
>> In particular, I would like to understand, for an eg DWordIO descriptor,
>> what Range Minimum, Range Maximum and Translation Offset represent,
>> they can't mean different things depending on the SW parsing them,
>> this totally defeats the purpose.
>
> I have no clue about what those mean in ACPI though.
>
> Generally speaking, each PCI domain is expected to have a (normally 64KB)
> range of CPU addresses that gets translated into PCI I/O space the same
> way that config space and memory space are handled.
> This is true for almost every architecture except for x86, which uses
> different CPU instructions for I/O space compared to the other spaces.
>
>> By the way, ia64 ioremaps the translation_offset (ie new_space()), so
>> basically that's the CPU physical address at which the PCI host bridge
>> map the IO space transactions), I do not think ia64 is any different from
>> arm64 in this respect, if it is please provide an HW description here from
>> the PCI bus perspective here (also an example of ia64 ACPI PCI host bridge
>> tables would help).
>
> The main difference between ia64 and a lot of the other architectures (e.g.
> sparc is different again) is that ia64 defines a logical address range
> in terms of having a small number for each I/O space followed by the
> offset within that space as a 'port number' and uses a mapping function
> that is defined as
>
> static inline void *__ia64_mk_io_addr (unsigned long port)
> {
> struct io_space *space = &io_space[IO_SPACE_NR(port)];
> return (space->mmio_base | IO_SPACE_PORT(port););
> }
> static inline unsigned int inl(unsigned long port)
> {
> return *__ia64_mk_io_addr(port);
> }
>
> Most architectures allow only one I/O port range and put it at a fixed
> virtual address so that inl() simply becomes
>
> static inline u32 inl(unsigned long addr)
> {
> return readl(PCI_IOBASE + addr);
> }
>
> which noticeably reduces code size.
>
> On some architectures (powerpc, arm, arm64), we then get the same simplified
> definition with a fixed virtual address, and use pci_ioremap_io() or
> something like that to to map a physical address range into this virtual
> address window at the correct io_offset;
Hi all,
Thanks for explanation, I found a way to make the ACPI resource
parsing interface arch neutral, it should help to address Lorenzo's
concern. Please refer to the attached patch. (It's still RFC, not tested
yet).
Thanks,
Gerry
On Tue, Nov 10, 2015 at 01:50:46PM +0800, Jiang Liu wrote:
[...]
> >> In particular, I would like to understand, for an eg DWordIO descriptor,
> >> what Range Minimum, Range Maximum and Translation Offset represent,
> >> they can't mean different things depending on the SW parsing them,
> >> this totally defeats the purpose.
> >
> > I have no clue about what those mean in ACPI though.
> >
> > Generally speaking, each PCI domain is expected to have a (normally 64KB)
> > range of CPU addresses that gets translated into PCI I/O space the same
> > way that config space and memory space are handled.
> > This is true for almost every architecture except for x86, which uses
> > different CPU instructions for I/O space compared to the other spaces.
> >
> >> By the way, ia64 ioremaps the translation_offset (ie new_space()), so
> >> basically that's the CPU physical address at which the PCI host bridge
> >> map the IO space transactions), I do not think ia64 is any different from
> >> arm64 in this respect, if it is please provide an HW description here from
> >> the PCI bus perspective here (also an example of ia64 ACPI PCI host bridge
> >> tables would help).
> >
> > The main difference between ia64 and a lot of the other architectures (e.g.
> > sparc is different again) is that ia64 defines a logical address range
> > in terms of having a small number for each I/O space followed by the
> > offset within that space as a 'port number' and uses a mapping function
> > that is defined as
> >
> > static inline void *__ia64_mk_io_addr (unsigned long port)
> > {
> > struct io_space *space = &io_space[IO_SPACE_NR(port)];
> > return (space->mmio_base | IO_SPACE_PORT(port););
> > }
> > static inline unsigned int inl(unsigned long port)
> > {
> > return *__ia64_mk_io_addr(port);
> > }
> >
> > Most architectures allow only one I/O port range and put it at a fixed
> > virtual address so that inl() simply becomes
> >
> > static inline u32 inl(unsigned long addr)
> > {
> > return readl(PCI_IOBASE + addr);
> > }
> >
> > which noticeably reduces code size.
> >
> > On some architectures (powerpc, arm, arm64), we then get the same simplified
> > definition with a fixed virtual address, and use pci_ioremap_io() or
> > something like that to to map a physical address range into this virtual
> > address window at the correct io_offset;
> Hi all,
> Thanks for explanation, I found a way to make the ACPI resource
> parsing interface arch neutral, it should help to address Lorenzo's
> concern. Please refer to the attached patch. (It's still RFC, not tested
> yet).
If we go with this approach though, you are not adding the offset to
the resource when parsing the memory spaces in acpi_decode_space(), are we
sure that's what we really want ?
In DT, a host bridge range has a:
- CPU physical address
- PCI bus address
We use that to compute the offset between primary bus (ie CPU physical
address) and secondary bus (ie PCI bus address).
The value ending up in the PCI resource struct (for memory space) is
the CPU physical address, if you do not add the offset in acpi_decode_space
that does not hold true on platforms where CPU<->PCI offset != 0 on ACPI,
am I wrong ?
Overall I think the point is related to ioport_resource and its check
in acpi_pci_root_validate_resources() which basically that's the
problem that started this thread.
On arm64, IO_SPACE_LIMIT is 16M, which, AFAIK is a kernel limit, not
a HW one. Comparing the resources parsed from the PCI bridge _CRS against
the range 0..IO_SPACE_LIMIT is not necessarily meaningful (or at least
not meaningful in its current form), on ia64 it works because IO_SPACE_LIMIT
is bumped up to 4G, that's the reason why adding the offset to the ACPI IO
resources work on ia64 as far as I understand.
And that's why I pulled Arnd in this discussion since he knows better
than me: what does ioport_resource _really_ represent on ARM64 ? It seems
to me that it is a range of IO ports values (ie a window that defines
the allowed offset in the virtual address space allocated to PCI IO) that
has _nothing_ to do with the CPU physical address at which the IO space is
actually mapped.
To sum it up for a, say, DWordIo/Memory descriptor:
- AddressMinimum, AddressMaximum represent the PCI bus addresses defining
the resource start..end
- AddressTranslation is the offset that has to be added to AddressMinimum
and AddressMaximum to get the window in CPU physical address space
So:
- Either we go with the patch attached (but please check my comment on
the memory spaces)
- Or we patch acpi_pci_root_validate_resources() to amend the way
IORESOURCE_IO is currently checked against ioport_resource, it can't
work on arm64 at present, I described why above
Thoughts appreciated it is time we got this sorted and thanks for
the patch.
Thanks,
Lorenzo
On Wed, Nov 11, 2015 at 05:46:47PM +0000, Lorenzo Pieralisi wrote:
> On Tue, Nov 10, 2015 at 01:50:46PM +0800, Jiang Liu wrote:
>
> [...]
>
> > >> In particular, I would like to understand, for an eg DWordIO descriptor,
> > >> what Range Minimum, Range Maximum and Translation Offset represent,
> > >> they can't mean different things depending on the SW parsing them,
> > >> this totally defeats the purpose.
> > >
> > > I have no clue about what those mean in ACPI though.
> > >
> > > Generally speaking, each PCI domain is expected to have a (normally 64KB)
> > > range of CPU addresses that gets translated into PCI I/O space the same
> > > way that config space and memory space are handled.
> > > This is true for almost every architecture except for x86, which uses
> > > different CPU instructions for I/O space compared to the other spaces.
> > >
> > >> By the way, ia64 ioremaps the translation_offset (ie new_space()), so
> > >> basically that's the CPU physical address at which the PCI host bridge
> > >> map the IO space transactions), I do not think ia64 is any different from
> > >> arm64 in this respect, if it is please provide an HW description here from
> > >> the PCI bus perspective here (also an example of ia64 ACPI PCI host bridge
> > >> tables would help).
> > >
> > > The main difference between ia64 and a lot of the other architectures (e.g.
> > > sparc is different again) is that ia64 defines a logical address range
> > > in terms of having a small number for each I/O space followed by the
> > > offset within that space as a 'port number' and uses a mapping function
> > > that is defined as
> > >
> > > static inline void *__ia64_mk_io_addr (unsigned long port)
> > > {
> > > struct io_space *space = &io_space[IO_SPACE_NR(port)];
> > > return (space->mmio_base | IO_SPACE_PORT(port););
> > > }
> > > static inline unsigned int inl(unsigned long port)
> > > {
> > > return *__ia64_mk_io_addr(port);
> > > }
> > >
> > > Most architectures allow only one I/O port range and put it at a fixed
> > > virtual address so that inl() simply becomes
> > >
> > > static inline u32 inl(unsigned long addr)
> > > {
> > > return readl(PCI_IOBASE + addr);
> > > }
> > >
> > > which noticeably reduces code size.
> > >
> > > On some architectures (powerpc, arm, arm64), we then get the same simplified
> > > definition with a fixed virtual address, and use pci_ioremap_io() or
> > > something like that to to map a physical address range into this virtual
> > > address window at the correct io_offset;
> > Hi all,
> > Thanks for explanation, I found a way to make the ACPI resource
> > parsing interface arch neutral, it should help to address Lorenzo's
> > concern. Please refer to the attached patch. (It's still RFC, not tested
> > yet).
>
> If we go with this approach though, you are not adding the offset to
> the resource when parsing the memory spaces in acpi_decode_space(), are we
> sure that's what we really want ?
>
> In DT, a host bridge range has a:
>
> - CPU physical address
> - PCI bus address
>
> We use that to compute the offset between primary bus (ie CPU physical
> address) and secondary bus (ie PCI bus address).
>
> The value ending up in the PCI resource struct (for memory space) is
> the CPU physical address, if you do not add the offset in acpi_decode_space
> that does not hold true on platforms where CPU<->PCI offset != 0 on ACPI,
> am I wrong ?
>
> Overall I think the point is related to ioport_resource and its check
> in acpi_pci_root_validate_resources() which basically that's the
> problem that started this thread.
>
> On arm64, IO_SPACE_LIMIT is 16M, which, AFAIK is a kernel limit, not
> a HW one.
You're right, it is a kernel limit. There is no HW limit for IO on arm64.
> Comparing the resources parsed from the PCI bridge _CRS against
> the range 0..IO_SPACE_LIMIT is not necessarily meaningful (or at least
> not meaningful in its current form), on ia64 it works because IO_SPACE_LIMIT
> is bumped up to 4G, that's the reason why adding the offset to the ACPI IO
> resources work on ia64 as far as I understand.
>
> And that's why I pulled Arnd in this discussion since he knows better
> than me: what does ioport_resource _really_ represent on ARM64 ? It seems
> to me that it is a range of IO ports values (ie a window that defines
> the allowed offset in the virtual address space allocated to PCI IO) that
> has _nothing_ to do with the CPU physical address at which the IO space is
> actually mapped.
Correct. The idea is that you can have any number of host bridges and what
you get back for an ioport_resource is a window inside the host bridge IO
space. That space is also offset-ed inside the kernel's IO port space
(0..IO_SPACE_LIMIT) by the amount of space already taken by preceeding
host bridges, so that two ioport_resources comming from two different
host bridges don't overlap in the CPU address space.
Best regards,
Liviu
>
> To sum it up for a, say, DWordIo/Memory descriptor:
>
> - AddressMinimum, AddressMaximum represent the PCI bus addresses defining
> the resource start..end
> - AddressTranslation is the offset that has to be added to AddressMinimum
> and AddressMaximum to get the window in CPU physical address space
>
> So:
>
> - Either we go with the patch attached (but please check my comment on
> the memory spaces)
> - Or we patch acpi_pci_root_validate_resources() to amend the way
> IORESOURCE_IO is currently checked against ioport_resource, it can't
> work on arm64 at present, I described why above
>
> Thoughts appreciated it is time we got this sorted and thanks for
> the patch.
>
> Thanks,
> Lorenzo
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
On Wednesday 11 November 2015 17:46:47 Lorenzo Pieralisi wrote:
> On Tue, Nov 10, 2015 at 01:50:46PM +0800, Jiang Liu wrote:
> If we go with this approach though, you are not adding the offset to
> the resource when parsing the memory spaces in acpi_decode_space(), are we
> sure that's what we really want ?
>
> In DT, a host bridge range has a:
>
> - CPU physical address
> - PCI bus address
>
> We use that to compute the offset between primary bus (ie CPU physical
> address) and secondary bus (ie PCI bus address).
>
> The value ending up in the PCI resource struct (for memory space) is
> the CPU physical address, if you do not add the offset in acpi_decode_space
> that does not hold true on platforms where CPU<->PCI offset != 0 on ACPI,
> am I wrong ?
Sinan Kaya pointed out that SBSA mandates a 1:1 mapping for memory space.
> On arm64, IO_SPACE_LIMIT is 16M, which, AFAIK is a kernel limit, not
> a HW one. Comparing the resources parsed from the PCI bridge _CRS against
> the range 0..IO_SPACE_LIMIT is not necessarily meaningful (or at least
> not meaningful in its current form), on ia64 it works because IO_SPACE_LIMIT
> is bumped up to 4G, that's the reason why adding the offset to the ACPI IO
> resources work on ia64 as far as I understand.
>
> And that's why I pulled Arnd in this discussion since he knows better
> than me: what does ioport_resource _really_ represent on ARM64 ? It seems
> to me that it is a range of IO ports values (ie a window that defines
> the allowed offset in the virtual address space allocated to PCI IO) that
> has _nothing_ to do with the CPU physical address at which the IO space is
> actually mapped.
Right, the ioport_resource uses the same space as the CPU virtual address,
with an offset of PCI_IOBASE that is defined as
#define PCI_IOBASE ((void __iomem *)PCI_IO_VIRT_BASE)
#define PCI_IO_START (PCI_IO_END - PCI_IO_SIZE)
#define PCI_IO_END (MODULES_VADDR - SZ_2M)
#define PCI_IO_SIZE SZ_16M
> To sum it up for a, say, DWordIo/Memory descriptor:
>
> - AddressMinimum, AddressMaximum represent the PCI bus addresses defining
> the resource start..end
> - AddressTranslation is the offset that has to be added to AddressMinimum
> and AddressMaximum to get the window in CPU physical address space
>
> So:
>
> - Either we go with the patch attached (but please check my comment on
> the memory spaces)
> - Or we patch acpi_pci_root_validate_resources() to amend the way
> IORESOURCE_IO is currently checked against ioport_resource, it can't
> work on arm64 at present, I described why above
>
> Thoughts appreciated it is time we got this sorted and thanks for
> the patch.
The easiest way would be to assume that nobody is building a server
system that has multiple I/O spaces. SBSA explicitly makes I/O space
optional, and really the only reason anyone includes that feature these
days is for initializing GPUs through the BIOS POST method, so that
is not relevant for servers. Even when you do have multiple PCIe
host controllers that all support I/O space, the most logical implementation
would be to share one common space.
If that fails, there are still two cases you have to care about, and
it really depends on what hardware makers decide to use here (and whether
we have any influence over them). The easier way for us to do this is
if every hardware sets up the mapping between CPU physical and PCI I/O
in a way that the I/O space numbers are non-overlapping between the
host controllers. That way we can still make Linux ioport_resource
addresses the same as PCI I/O space addresses (using io_offset=0),
with the downside being that only the first PCIe host (as enumerated
by the bootloader) can have I/O space addresses below 1024 that may
be required for ISA compatibility on some hardware.
Arnd
On 2015/11/12 1:46, Lorenzo Pieralisi wrote:
> On Tue, Nov 10, 2015 at 01:50:46PM +0800, Jiang Liu wrote:
>
> [...]
>
>>>> In particular, I would like to understand, for an eg DWordIO descriptor,
>>>> what Range Minimum, Range Maximum and Translation Offset represent,
>>>> they can't mean different things depending on the SW parsing them,
>>>> this totally defeats the purpose.
>>>
>>> I have no clue about what those mean in ACPI though.
>>>
>>> Generally speaking, each PCI domain is expected to have a (normally 64KB)
>>> range of CPU addresses that gets translated into PCI I/O space the same
>>> way that config space and memory space are handled.
>>> This is true for almost every architecture except for x86, which uses
>>> different CPU instructions for I/O space compared to the other spaces.
>>>
>>>> By the way, ia64 ioremaps the translation_offset (ie new_space()), so
>>>> basically that's the CPU physical address at which the PCI host bridge
>>>> map the IO space transactions), I do not think ia64 is any different from
>>>> arm64 in this respect, if it is please provide an HW description here from
>>>> the PCI bus perspective here (also an example of ia64 ACPI PCI host bridge
>>>> tables would help).
>>>
>>> The main difference between ia64 and a lot of the other architectures (e.g.
>>> sparc is different again) is that ia64 defines a logical address range
>>> in terms of having a small number for each I/O space followed by the
>>> offset within that space as a 'port number' and uses a mapping function
>>> that is defined as
>>>
>>> static inline void *__ia64_mk_io_addr (unsigned long port)
>>> {
>>> struct io_space *space = &io_space[IO_SPACE_NR(port)];
>>> return (space->mmio_base | IO_SPACE_PORT(port););
>>> }
>>> static inline unsigned int inl(unsigned long port)
>>> {
>>> return *__ia64_mk_io_addr(port);
>>> }
>>>
>>> Most architectures allow only one I/O port range and put it at a fixed
>>> virtual address so that inl() simply becomes
>>>
>>> static inline u32 inl(unsigned long addr)
>>> {
>>> return readl(PCI_IOBASE + addr);
>>> }
>>>
>>> which noticeably reduces code size.
>>>
>>> On some architectures (powerpc, arm, arm64), we then get the same simplified
>>> definition with a fixed virtual address, and use pci_ioremap_io() or
>>> something like that to to map a physical address range into this virtual
>>> address window at the correct io_offset;
>> Hi all,
>> Thanks for explanation, I found a way to make the ACPI resource
>> parsing interface arch neutral, it should help to address Lorenzo's
>> concern. Please refer to the attached patch. (It's still RFC, not tested
>> yet).
>
> If we go with this approach though, you are not adding the offset to
> the resource when parsing the memory spaces in acpi_decode_space(), are we
> sure that's what we really want ?
>
> In DT, a host bridge range has a:
>
> - CPU physical address
> - PCI bus address
>
> We use that to compute the offset between primary bus (ie CPU physical
> address) and secondary bus (ie PCI bus address).
>
> The value ending up in the PCI resource struct (for memory space) is
> the CPU physical address, if you do not add the offset in acpi_decode_space
> that does not hold true on platforms where CPU<->PCI offset != 0 on ACPI,
> am I wrong ?
Hi Lorenzo,
I may have found the divergence between us about the design here. You
treat it as a one-stage translation but I treat it as a
two-stage translation as below:
stage 1: map(translate) per-PCI-domain IO port address[0, 16M) into
system global IO port address. Here system global IO port address is
ioport_resource[0, IO_SPACE_LIMIT).
stage 2: map system IO port address into system memory address.
We need two objects of struct resource_win to support above two-stage
translation. One object, type of IORESOURCE_IO, is used to support
stage one, and it will also used to allocate IO port resources
for PCI devices. Another object, type of IORESOURCE_MMIO, is used
to allocate resource from iomem_resource and setup MMIO mapping
to actually access IO ports.
For ARM64, it doesn't support multiple per-PCI-domain(bus local)
IO port address space yet, so stage one seems to be optional
becomes the offset between bus local IO port address and system
IO port address is always 0. But we still need two objects of
struct resource_win. The first object is
{
offset:0,
start:AddressMinimum,
end:AddressMaximum,
flags:IORESOURCE_IO
}
Here it's type of IORESOURCE_IO and offset must be zero because
pcibios_resource_to_bus() will access it translate system IO
port address into bus local IO port address. With my patch,
the struct resource_win object created by the ACPI core will
be reused for this.
The second object is:
{
offset:Translation_Offset,
start:AddressMinimum + Translation_Offset,
end:AddressMaximum + Translation_Offset,
flags:IORESOURCE_MMIO
}
Arch code need to create the second struct resource_win object
and actually setup the MMIO mapping.
But there's really another bug need to get fixed, funciton
acpi_dev_ioresource_flags() assumes bus local IO port address
space is size of 64K, which is wrong for IA64 and ARM64.
Thanks,
Gerry
>
> Overall I think the point is related to ioport_resource and its check
> in acpi_pci_root_validate_resources() which basically that's the
> problem that started this thread.
>
> On arm64, IO_SPACE_LIMIT is 16M, which, AFAIK is a kernel limit, not
> a HW one. Comparing the resources parsed from the PCI bridge _CRS against
> the range 0..IO_SPACE_LIMIT is not necessarily meaningful (or at least
> not meaningful in its current form), on ia64 it works because IO_SPACE_LIMIT
> is bumped up to 4G, that's the reason why adding the offset to the ACPI IO
> resources work on ia64 as far as I understand.
>
> And that's why I pulled Arnd in this discussion since he knows better
> than me: what does ioport_resource _really_ represent on ARM64 ? It seems
> to me that it is a range of IO ports values (ie a window that defines
> the allowed offset in the virtual address space allocated to PCI IO) that
> has _nothing_ to do with the CPU physical address at which the IO space is
> actually mapped.
>
> To sum it up for a, say, DWordIo/Memory descriptor:
>
> - AddressMinimum, AddressMaximum represent the PCI bus addresses defining
> the resource start..end
> - AddressTranslation is the offset that has to be added to AddressMinimum
> and AddressMaximum to get the window in CPU physical address space
>
> So:
>
> - Either we go with the patch attached (but please check my comment on
> the memory spaces)
> - Or we patch acpi_pci_root_validate_resources() to amend the way
> IORESOURCE_IO is currently checked against ioport_resource, it can't
> work on arm64 at present, I described why above
>
> Thoughts appreciated it is time we got this sorted and thanks for
> the patch.
>
> Thanks,
> Lorenzo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Wed, Nov 11, 2015 at 09:55:37PM +0100, Arnd Bergmann wrote:
> On Wednesday 11 November 2015 17:46:47 Lorenzo Pieralisi wrote:
> > On Tue, Nov 10, 2015 at 01:50:46PM +0800, Jiang Liu wrote:
> > If we go with this approach though, you are not adding the offset to
> > the resource when parsing the memory spaces in acpi_decode_space(), are we
> > sure that's what we really want ?
> >
> > In DT, a host bridge range has a:
> >
> > - CPU physical address
> > - PCI bus address
> >
> > We use that to compute the offset between primary bus (ie CPU physical
> > address) and secondary bus (ie PCI bus address).
> >
> > The value ending up in the PCI resource struct (for memory space) is
> > the CPU physical address, if you do not add the offset in acpi_decode_space
> > that does not hold true on platforms where CPU<->PCI offset != 0 on ACPI,
> > am I wrong ?
>
> Sinan Kaya pointed out that SBSA mandates a 1:1 mapping for memory space.
acpi_decode_space() is generic code though, it has to work on all arches,
I was worried about triggering regressions on x86 and ia64 here.
[...]
> > To sum it up for a, say, DWordIo/Memory descriptor:
> >
> > - AddressMinimum, AddressMaximum represent the PCI bus addresses defining
> > the resource start..end
> > - AddressTranslation is the offset that has to be added to AddressMinimum
> > and AddressMaximum to get the window in CPU physical address space
> >
> > So:
> >
> > - Either we go with the patch attached (but please check my comment on
> > the memory spaces)
> > - Or we patch acpi_pci_root_validate_resources() to amend the way
> > IORESOURCE_IO is currently checked against ioport_resource, it can't
> > work on arm64 at present, I described why above
> >
> > Thoughts appreciated it is time we got this sorted and thanks for
> > the patch.
>
> The easiest way would be to assume that nobody is building a server
> system that has multiple I/O spaces. SBSA explicitly makes I/O space
> optional, and really the only reason anyone includes that feature these
> days is for initializing GPUs through the BIOS POST method, so that
> is not relevant for servers. Even when you do have multiple PCIe
> host controllers that all support I/O space, the most logical implementation
> would be to share one common space.
>
> If that fails, there are still two cases you have to care about, and
> it really depends on what hardware makers decide to use here (and whether
> we have any influence over them). The easier way for us to do this is
> if every hardware sets up the mapping between CPU physical and PCI I/O
> in a way that the I/O space numbers are non-overlapping between the
> host controllers. That way we can still make Linux ioport_resource
> addresses the same as PCI I/O space addresses (using io_offset=0),
> with the downside being that only the first PCIe host (as enumerated
> by the bootloader) can have I/O space addresses below 1024 that may
> be required for ISA compatibility on some hardware.
Yes, I think the approach we have in place on arm64 sound, I will work with
Jiang to make sure we interpret and manage IO space on arm64 the same
way we do with DT, I do not expect any issue on that side, I was more
worried about the interpretation of ACPI tables, I really really do not
want to end up with ACPI tables that are set-up in a way that can cause
regressions, we have to agree (and make it crystal clear in the ACPI
specs) what the resource descriptors define and how, then the ACPI kernel
resource layer should be made compliant.
Thanks,
Lorenzo
On 12.11.2015 09:43, Jiang Liu wrote:
> On 2015/11/12 1:46, Lorenzo Pieralisi wrote:
>> On Tue, Nov 10, 2015 at 01:50:46PM +0800, Jiang Liu wrote:
>>
>> [...]
>>
>>>>> In particular, I would like to understand, for an eg DWordIO descriptor,
>>>>> what Range Minimum, Range Maximum and Translation Offset represent,
>>>>> they can't mean different things depending on the SW parsing them,
>>>>> this totally defeats the purpose.
>>>>
>>>> I have no clue about what those mean in ACPI though.
>>>>
>>>> Generally speaking, each PCI domain is expected to have a (normally 64KB)
>>>> range of CPU addresses that gets translated into PCI I/O space the same
>>>> way that config space and memory space are handled.
>>>> This is true for almost every architecture except for x86, which uses
>>>> different CPU instructions for I/O space compared to the other spaces.
>>>>
>>>>> By the way, ia64 ioremaps the translation_offset (ie new_space()), so
>>>>> basically that's the CPU physical address at which the PCI host bridge
>>>>> map the IO space transactions), I do not think ia64 is any different from
>>>>> arm64 in this respect, if it is please provide an HW description here from
>>>>> the PCI bus perspective here (also an example of ia64 ACPI PCI host bridge
>>>>> tables would help).
>>>>
>>>> The main difference between ia64 and a lot of the other architectures (e.g.
>>>> sparc is different again) is that ia64 defines a logical address range
>>>> in terms of having a small number for each I/O space followed by the
>>>> offset within that space as a 'port number' and uses a mapping function
>>>> that is defined as
>>>>
>>>> static inline void *__ia64_mk_io_addr (unsigned long port)
>>>> {
>>>> struct io_space *space = &io_space[IO_SPACE_NR(port)];
>>>> return (space->mmio_base | IO_SPACE_PORT(port););
>>>> }
>>>> static inline unsigned int inl(unsigned long port)
>>>> {
>>>> return *__ia64_mk_io_addr(port);
>>>> }
>>>>
>>>> Most architectures allow only one I/O port range and put it at a fixed
>>>> virtual address so that inl() simply becomes
>>>>
>>>> static inline u32 inl(unsigned long addr)
>>>> {
>>>> return readl(PCI_IOBASE + addr);
>>>> }
>>>>
>>>> which noticeably reduces code size.
>>>>
>>>> On some architectures (powerpc, arm, arm64), we then get the same simplified
>>>> definition with a fixed virtual address, and use pci_ioremap_io() or
>>>> something like that to to map a physical address range into this virtual
>>>> address window at the correct io_offset;
>>> Hi all,
>>> Thanks for explanation, I found a way to make the ACPI resource
>>> parsing interface arch neutral, it should help to address Lorenzo's
>>> concern. Please refer to the attached patch. (It's still RFC, not tested
>>> yet).
>>
>> If we go with this approach though, you are not adding the offset to
>> the resource when parsing the memory spaces in acpi_decode_space(), are we
>> sure that's what we really want ?
>>
>> In DT, a host bridge range has a:
>>
>> - CPU physical address
>> - PCI bus address
>>
>> We use that to compute the offset between primary bus (ie CPU physical
>> address) and secondary bus (ie PCI bus address).
>>
>> The value ending up in the PCI resource struct (for memory space) is
>> the CPU physical address, if you do not add the offset in acpi_decode_space
>> that does not hold true on platforms where CPU<->PCI offset != 0 on ACPI,
>> am I wrong ?
> Hi Lorenzo,
> I may have found the divergence between us about the design here. You
> treat it as a one-stage translation but I treat it as a
> two-stage translation as below:
> stage 1: map(translate) per-PCI-domain IO port address[0, 16M) into
> system global IO port address. Here system global IO port address is
> ioport_resource[0, IO_SPACE_LIMIT).
> stage 2: map system IO port address into system memory address.
>
> We need two objects of struct resource_win to support above two-stage
> translation. One object, type of IORESOURCE_IO, is used to support
> stage one, and it will also used to allocate IO port resources
> for PCI devices. Another object, type of IORESOURCE_MMIO, is used
> to allocate resource from iomem_resource and setup MMIO mapping
> to actually access IO ports.
>
> For ARM64, it doesn't support multiple per-PCI-domain(bus local)
> IO port address space yet, so stage one seems to be optional
> becomes the offset between bus local IO port address and system
> IO port address is always 0. But we still need two objects of
> struct resource_win. The first object is
> {
> offset:0,
> start:AddressMinimum,
> end:AddressMaximum,
> flags:IORESOURCE_IO
> }
> Here it's type of IORESOURCE_IO and offset must be zero because
> pcibios_resource_to_bus() will access it translate system IO
> port address into bus local IO port address. With my patch,
> the struct resource_win object created by the ACPI core will
> be reused for this.
>
> The second object is:
> {
> offset:Translation_Offset,
> start:AddressMinimum + Translation_Offset,
> end:AddressMaximum + Translation_Offset,
> flags:IORESOURCE_MMIO
> }
> Arch code need to create the second struct resource_win object
> and actually setup the MMIO mapping.
>
> But there's really another bug need to get fixed, funciton
> acpi_dev_ioresource_flags() assumes bus local IO port address
> space is size of 64K, which is wrong for IA64 and ARM64.
>
So what would be the Translation_Offset meaning for two cases DWordIo
(....,TypeTranslation) vs DWordIo (....,TypeStatic)? And why we did not
use TypeTranslation for IA64 so far?
I am worried that TypeTranslation fall into the IA64 category but ACPI
tables were already written incorrectly.
Tomasz
On 2015/11/12 21:21, Tomasz Nowicki wrote:
> On 12.11.2015 09:43, Jiang Liu wrote:
>> On 2015/11/12 1:46, Lorenzo Pieralisi wrote:
>>> On Tue, Nov 10, 2015 at 01:50:46PM +0800, Jiang Liu wrote:
>>>
>>> [...]
>>>
>>>>>> In particular, I would like to understand, for an eg DWordIO
>>>>>> descriptor,
>>>>>> what Range Minimum, Range Maximum and Translation Offset represent,
>>>>>> they can't mean different things depending on the SW parsing them,
>>>>>> this totally defeats the purpose.
>>>>>
>>>>> I have no clue about what those mean in ACPI though.
>>>>>
>>>>> Generally speaking, each PCI domain is expected to have a (normally
>>>>> 64KB)
>>>>> range of CPU addresses that gets translated into PCI I/O space the
>>>>> same
>>>>> way that config space and memory space are handled.
>>>>> This is true for almost every architecture except for x86, which uses
>>>>> different CPU instructions for I/O space compared to the other spaces.
>>>>>
>>>>>> By the way, ia64 ioremaps the translation_offset (ie new_space()), so
>>>>>> basically that's the CPU physical address at which the PCI host
>>>>>> bridge
>>>>>> map the IO space transactions), I do not think ia64 is any
>>>>>> different from
>>>>>> arm64 in this respect, if it is please provide an HW description
>>>>>> here from
>>>>>> the PCI bus perspective here (also an example of ia64 ACPI PCI
>>>>>> host bridge
>>>>>> tables would help).
>>>>>
>>>>> The main difference between ia64 and a lot of the other
>>>>> architectures (e.g.
>>>>> sparc is different again) is that ia64 defines a logical address range
>>>>> in terms of having a small number for each I/O space followed by the
>>>>> offset within that space as a 'port number' and uses a mapping
>>>>> function
>>>>> that is defined as
>>>>>
>>>>> static inline void *__ia64_mk_io_addr (unsigned long port)
>>>>> {
>>>>> struct io_space *space = &io_space[IO_SPACE_NR(port)];
>>>>> return (space->mmio_base | IO_SPACE_PORT(port););
>>>>> }
>>>>> static inline unsigned int inl(unsigned long port)
>>>>> {
>>>>> return *__ia64_mk_io_addr(port);
>>>>> }
>>>>>
>>>>> Most architectures allow only one I/O port range and put it at a fixed
>>>>> virtual address so that inl() simply becomes
>>>>>
>>>>> static inline u32 inl(unsigned long addr)
>>>>> {
>>>>> return readl(PCI_IOBASE + addr);
>>>>> }
>>>>>
>>>>> which noticeably reduces code size.
>>>>>
>>>>> On some architectures (powerpc, arm, arm64), we then get the same
>>>>> simplified
>>>>> definition with a fixed virtual address, and use pci_ioremap_io() or
>>>>> something like that to to map a physical address range into this
>>>>> virtual
>>>>> address window at the correct io_offset;
>>>> Hi all,
>>>> Thanks for explanation, I found a way to make the ACPI resource
>>>> parsing interface arch neutral, it should help to address Lorenzo's
>>>> concern. Please refer to the attached patch. (It's still RFC, not
>>>> tested
>>>> yet).
>>>
>>> If we go with this approach though, you are not adding the offset to
>>> the resource when parsing the memory spaces in acpi_decode_space(),
>>> are we
>>> sure that's what we really want ?
>>>
>>> In DT, a host bridge range has a:
>>>
>>> - CPU physical address
>>> - PCI bus address
>>>
>>> We use that to compute the offset between primary bus (ie CPU physical
>>> address) and secondary bus (ie PCI bus address).
>>>
>>> The value ending up in the PCI resource struct (for memory space) is
>>> the CPU physical address, if you do not add the offset in
>>> acpi_decode_space
>>> that does not hold true on platforms where CPU<->PCI offset != 0 on
>>> ACPI,
>>> am I wrong ?
>> Hi Lorenzo,
>> I may have found the divergence between us about the design here. You
>> treat it as a one-stage translation but I treat it as a
>> two-stage translation as below:
>> stage 1: map(translate) per-PCI-domain IO port address[0, 16M) into
>> system global IO port address. Here system global IO port address is
>> ioport_resource[0, IO_SPACE_LIMIT).
>> stage 2: map system IO port address into system memory address.
>>
>> We need two objects of struct resource_win to support above two-stage
>> translation. One object, type of IORESOURCE_IO, is used to support
>> stage one, and it will also used to allocate IO port resources
>> for PCI devices. Another object, type of IORESOURCE_MMIO, is used
>> to allocate resource from iomem_resource and setup MMIO mapping
>> to actually access IO ports.
>>
>> For ARM64, it doesn't support multiple per-PCI-domain(bus local)
>> IO port address space yet, so stage one seems to be optional
>> becomes the offset between bus local IO port address and system
>> IO port address is always 0. But we still need two objects of
>> struct resource_win. The first object is
>> {
>> offset:0,
>> start:AddressMinimum,
>> end:AddressMaximum,
>> flags:IORESOURCE_IO
>> }
>> Here it's type of IORESOURCE_IO and offset must be zero because
>> pcibios_resource_to_bus() will access it translate system IO
>> port address into bus local IO port address. With my patch,
>> the struct resource_win object created by the ACPI core will
>> be reused for this.
>>
>> The second object is:
>> {
>> offset:Translation_Offset,
>> start:AddressMinimum + Translation_Offset,
>> end:AddressMaximum + Translation_Offset,
>> flags:IORESOURCE_MMIO
>> }
>> Arch code need to create the second struct resource_win object
>> and actually setup the MMIO mapping.
>>
>> But there's really another bug need to get fixed, funciton
>> acpi_dev_ioresource_flags() assumes bus local IO port address
>> space is size of 64K, which is wrong for IA64 and ARM64.
>>
>
> So what would be the Translation_Offset meaning for two cases DWordIo
> (....,TypeTranslation) vs DWordIo (....,TypeStatic)? And why we did not
> use TypeTranslation for IA64 so far?
IA64 actually ignores the translation type flag and just assume it's
TypeTranslation, so there may be some IA64 BIOS implementations
accidentally using TypeStatic. That's why we parsing SparseTranslation
flag without checking TranslationType flag. I feel ARM64 may face the
same situation as IA64:(
We may expect (TypeStatic, 0-offset) and (TypeTranslation,
non-0-offset) in real word. For other two combinations, I haven't
found a real usage yet, though theoretically they are possible.
Thanks,
Gerry
>
> I am worried that TypeTranslation fall into the IA64 category but ACPI
> tables were already written incorrectly.
>
> Tomasz
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On 12.11.2015 15:04, Jiang Liu wrote:
> On 2015/11/12 21:21, Tomasz Nowicki wrote:
>> On 12.11.2015 09:43, Jiang Liu wrote:
>>> On 2015/11/12 1:46, Lorenzo Pieralisi wrote:
>>>> On Tue, Nov 10, 2015 at 01:50:46PM +0800, Jiang Liu wrote:
>>>>
>>>> [...]
>>>>
>>>>>>> In particular, I would like to understand, for an eg DWordIO
>>>>>>> descriptor,
>>>>>>> what Range Minimum, Range Maximum and Translation Offset represent,
>>>>>>> they can't mean different things depending on the SW parsing them,
>>>>>>> this totally defeats the purpose.
>>>>>>
>>>>>> I have no clue about what those mean in ACPI though.
>>>>>>
>>>>>> Generally speaking, each PCI domain is expected to have a (normally
>>>>>> 64KB)
>>>>>> range of CPU addresses that gets translated into PCI I/O space the
>>>>>> same
>>>>>> way that config space and memory space are handled.
>>>>>> This is true for almost every architecture except for x86, which uses
>>>>>> different CPU instructions for I/O space compared to the other spaces.
>>>>>>
>>>>>>> By the way, ia64 ioremaps the translation_offset (ie new_space()), so
>>>>>>> basically that's the CPU physical address at which the PCI host
>>>>>>> bridge
>>>>>>> map the IO space transactions), I do not think ia64 is any
>>>>>>> different from
>>>>>>> arm64 in this respect, if it is please provide an HW description
>>>>>>> here from
>>>>>>> the PCI bus perspective here (also an example of ia64 ACPI PCI
>>>>>>> host bridge
>>>>>>> tables would help).
>>>>>>
>>>>>> The main difference between ia64 and a lot of the other
>>>>>> architectures (e.g.
>>>>>> sparc is different again) is that ia64 defines a logical address range
>>>>>> in terms of having a small number for each I/O space followed by the
>>>>>> offset within that space as a 'port number' and uses a mapping
>>>>>> function
>>>>>> that is defined as
>>>>>>
>>>>>> static inline void *__ia64_mk_io_addr (unsigned long port)
>>>>>> {
>>>>>> struct io_space *space = &io_space[IO_SPACE_NR(port)];
>>>>>> return (space->mmio_base | IO_SPACE_PORT(port););
>>>>>> }
>>>>>> static inline unsigned int inl(unsigned long port)
>>>>>> {
>>>>>> return *__ia64_mk_io_addr(port);
>>>>>> }
>>>>>>
>>>>>> Most architectures allow only one I/O port range and put it at a fixed
>>>>>> virtual address so that inl() simply becomes
>>>>>>
>>>>>> static inline u32 inl(unsigned long addr)
>>>>>> {
>>>>>> return readl(PCI_IOBASE + addr);
>>>>>> }
>>>>>>
>>>>>> which noticeably reduces code size.
>>>>>>
>>>>>> On some architectures (powerpc, arm, arm64), we then get the same
>>>>>> simplified
>>>>>> definition with a fixed virtual address, and use pci_ioremap_io() or
>>>>>> something like that to to map a physical address range into this
>>>>>> virtual
>>>>>> address window at the correct io_offset;
>>>>> Hi all,
>>>>> Thanks for explanation, I found a way to make the ACPI resource
>>>>> parsing interface arch neutral, it should help to address Lorenzo's
>>>>> concern. Please refer to the attached patch. (It's still RFC, not
>>>>> tested
>>>>> yet).
>>>>
>>>> If we go with this approach though, you are not adding the offset to
>>>> the resource when parsing the memory spaces in acpi_decode_space(),
>>>> are we
>>>> sure that's what we really want ?
>>>>
>>>> In DT, a host bridge range has a:
>>>>
>>>> - CPU physical address
>>>> - PCI bus address
>>>>
>>>> We use that to compute the offset between primary bus (ie CPU physical
>>>> address) and secondary bus (ie PCI bus address).
>>>>
>>>> The value ending up in the PCI resource struct (for memory space) is
>>>> the CPU physical address, if you do not add the offset in
>>>> acpi_decode_space
>>>> that does not hold true on platforms where CPU<->PCI offset != 0 on
>>>> ACPI,
>>>> am I wrong ?
>>> Hi Lorenzo,
>>> I may have found the divergence between us about the design here. You
>>> treat it as a one-stage translation but I treat it as a
>>> two-stage translation as below:
>>> stage 1: map(translate) per-PCI-domain IO port address[0, 16M) into
>>> system global IO port address. Here system global IO port address is
>>> ioport_resource[0, IO_SPACE_LIMIT).
>>> stage 2: map system IO port address into system memory address.
>>>
>>> We need two objects of struct resource_win to support above two-stage
>>> translation. One object, type of IORESOURCE_IO, is used to support
>>> stage one, and it will also used to allocate IO port resources
>>> for PCI devices. Another object, type of IORESOURCE_MMIO, is used
>>> to allocate resource from iomem_resource and setup MMIO mapping
>>> to actually access IO ports.
>>>
>>> For ARM64, it doesn't support multiple per-PCI-domain(bus local)
>>> IO port address space yet, so stage one seems to be optional
>>> becomes the offset between bus local IO port address and system
>>> IO port address is always 0. But we still need two objects of
>>> struct resource_win. The first object is
>>> {
>>> offset:0,
>>> start:AddressMinimum,
>>> end:AddressMaximum,
>>> flags:IORESOURCE_IO
>>> }
>>> Here it's type of IORESOURCE_IO and offset must be zero because
>>> pcibios_resource_to_bus() will access it translate system IO
>>> port address into bus local IO port address. With my patch,
>>> the struct resource_win object created by the ACPI core will
>>> be reused for this.
>>>
>>> The second object is:
>>> {
>>> offset:Translation_Offset,
>>> start:AddressMinimum + Translation_Offset,
>>> end:AddressMaximum + Translation_Offset,
>>> flags:IORESOURCE_MMIO
>>> }
>>> Arch code need to create the second struct resource_win object
>>> and actually setup the MMIO mapping.
>>>
>>> But there's really another bug need to get fixed, funciton
>>> acpi_dev_ioresource_flags() assumes bus local IO port address
>>> space is size of 64K, which is wrong for IA64 and ARM64.
>>>
>>
>> So what would be the Translation_Offset meaning for two cases DWordIo
>> (....,TypeTranslation) vs DWordIo (....,TypeStatic)? And why we did not
>> use TypeTranslation for IA64 so far?
>
> IA64 actually ignores the translation type flag and just assume it's
> TypeTranslation, so there may be some IA64 BIOS implementations
> accidentally using TypeStatic. That's why we parsing SparseTranslation
> flag without checking TranslationType flag. I feel ARM64 may face the
> same situation as IA64:(
>
> We may expect (TypeStatic, 0-offset) and (TypeTranslation,
> non-0-offset) in real word. For other two combinations, I haven't
> found a real usage yet, though theoretically they are possible.
>
I think we should not bend the generic code for IA64 only and expose
other platforms to the same issue. Instead, lets interpret spec
correctly and create IA64 quirk for the sake of backward compatibility.
Thoughts?
Regards,
Tomasz
On 2015/11/12 22:45, Tomasz Nowicki wrote:
> On 12.11.2015 15:04, Jiang Liu wrote:
>> On 2015/11/12 21:21, Tomasz Nowicki wrote:
>>> On 12.11.2015 09:43, Jiang Liu wrote:
>>>> On 2015/11/12 1:46, Lorenzo Pieralisi wrote:
>>>>> On Tue, Nov 10, 2015 at 01:50:46PM +0800, Jiang Liu wrote:
>>>>>
>>>>> [...]
>>>>>
>>>>>>>> In particular, I would like to understand, for an eg DWordIO
>>>>>>>> descriptor,
>>>>>>>> what Range Minimum, Range Maximum and Translation Offset represent,
>>>>>>>> they can't mean different things depending on the SW parsing them,
>>>>>>>> this totally defeats the purpose.
>>>>>>>
>>>>>>> I have no clue about what those mean in ACPI though.
>>>>>>>
>>>>>>> Generally speaking, each PCI domain is expected to have a (normally
>>>>>>> 64KB)
>>>>>>> range of CPU addresses that gets translated into PCI I/O space the
>>>>>>> same
>>>>>>> way that config space and memory space are handled.
>>>>>>> This is true for almost every architecture except for x86, which
>>>>>>> uses
>>>>>>> different CPU instructions for I/O space compared to the other
>>>>>>> spaces.
>>>>>>>
>>>>>>>> By the way, ia64 ioremaps the translation_offset (ie
>>>>>>>> new_space()), so
>>>>>>>> basically that's the CPU physical address at which the PCI host
>>>>>>>> bridge
>>>>>>>> map the IO space transactions), I do not think ia64 is any
>>>>>>>> different from
>>>>>>>> arm64 in this respect, if it is please provide an HW description
>>>>>>>> here from
>>>>>>>> the PCI bus perspective here (also an example of ia64 ACPI PCI
>>>>>>>> host bridge
>>>>>>>> tables would help).
>>>>>>>
>>>>>>> The main difference between ia64 and a lot of the other
>>>>>>> architectures (e.g.
>>>>>>> sparc is different again) is that ia64 defines a logical address
>>>>>>> range
>>>>>>> in terms of having a small number for each I/O space followed by the
>>>>>>> offset within that space as a 'port number' and uses a mapping
>>>>>>> function
>>>>>>> that is defined as
>>>>>>>
>>>>>>> static inline void *__ia64_mk_io_addr (unsigned long port)
>>>>>>> {
>>>>>>> struct io_space *space = &io_space[IO_SPACE_NR(port)];
>>>>>>> return (space->mmio_base | IO_SPACE_PORT(port););
>>>>>>> }
>>>>>>> static inline unsigned int inl(unsigned long port)
>>>>>>> {
>>>>>>> return *__ia64_mk_io_addr(port);
>>>>>>> }
>>>>>>>
>>>>>>> Most architectures allow only one I/O port range and put it at a
>>>>>>> fixed
>>>>>>> virtual address so that inl() simply becomes
>>>>>>>
>>>>>>> static inline u32 inl(unsigned long addr)
>>>>>>> {
>>>>>>> return readl(PCI_IOBASE + addr);
>>>>>>> }
>>>>>>>
>>>>>>> which noticeably reduces code size.
>>>>>>>
>>>>>>> On some architectures (powerpc, arm, arm64), we then get the same
>>>>>>> simplified
>>>>>>> definition with a fixed virtual address, and use pci_ioremap_io() or
>>>>>>> something like that to to map a physical address range into this
>>>>>>> virtual
>>>>>>> address window at the correct io_offset;
>>>>>> Hi all,
>>>>>> Thanks for explanation, I found a way to make the ACPI resource
>>>>>> parsing interface arch neutral, it should help to address Lorenzo's
>>>>>> concern. Please refer to the attached patch. (It's still RFC, not
>>>>>> tested
>>>>>> yet).
>>>>>
>>>>> If we go with this approach though, you are not adding the offset to
>>>>> the resource when parsing the memory spaces in acpi_decode_space(),
>>>>> are we
>>>>> sure that's what we really want ?
>>>>>
>>>>> In DT, a host bridge range has a:
>>>>>
>>>>> - CPU physical address
>>>>> - PCI bus address
>>>>>
>>>>> We use that to compute the offset between primary bus (ie CPU physical
>>>>> address) and secondary bus (ie PCI bus address).
>>>>>
>>>>> The value ending up in the PCI resource struct (for memory space) is
>>>>> the CPU physical address, if you do not add the offset in
>>>>> acpi_decode_space
>>>>> that does not hold true on platforms where CPU<->PCI offset != 0 on
>>>>> ACPI,
>>>>> am I wrong ?
>>>> Hi Lorenzo,
>>>> I may have found the divergence between us about the design
>>>> here. You
>>>> treat it as a one-stage translation but I treat it as a
>>>> two-stage translation as below:
>>>> stage 1: map(translate) per-PCI-domain IO port address[0, 16M) into
>>>> system global IO port address. Here system global IO port address is
>>>> ioport_resource[0, IO_SPACE_LIMIT).
>>>> stage 2: map system IO port address into system memory address.
>>>>
>>>> We need two objects of struct resource_win to support above two-stage
>>>> translation. One object, type of IORESOURCE_IO, is used to support
>>>> stage one, and it will also used to allocate IO port resources
>>>> for PCI devices. Another object, type of IORESOURCE_MMIO, is used
>>>> to allocate resource from iomem_resource and setup MMIO mapping
>>>> to actually access IO ports.
>>>>
>>>> For ARM64, it doesn't support multiple per-PCI-domain(bus local)
>>>> IO port address space yet, so stage one seems to be optional
>>>> becomes the offset between bus local IO port address and system
>>>> IO port address is always 0. But we still need two objects of
>>>> struct resource_win. The first object is
>>>> {
>>>> offset:0,
>>>> start:AddressMinimum,
>>>> end:AddressMaximum,
>>>> flags:IORESOURCE_IO
>>>> }
>>>> Here it's type of IORESOURCE_IO and offset must be zero because
>>>> pcibios_resource_to_bus() will access it translate system IO
>>>> port address into bus local IO port address. With my patch,
>>>> the struct resource_win object created by the ACPI core will
>>>> be reused for this.
>>>>
>>>> The second object is:
>>>> {
>>>> offset:Translation_Offset,
>>>> start:AddressMinimum + Translation_Offset,
>>>> end:AddressMaximum + Translation_Offset,
>>>> flags:IORESOURCE_MMIO
>>>> }
>>>> Arch code need to create the second struct resource_win object
>>>> and actually setup the MMIO mapping.
>>>>
>>>> But there's really another bug need to get fixed, funciton
>>>> acpi_dev_ioresource_flags() assumes bus local IO port address
>>>> space is size of 64K, which is wrong for IA64 and ARM64.
>>>>
>>>
>>> So what would be the Translation_Offset meaning for two cases DWordIo
>>> (....,TypeTranslation) vs DWordIo (....,TypeStatic)? And why we did not
>>> use TypeTranslation for IA64 so far?
>>
>> IA64 actually ignores the translation type flag and just assume it's
>> TypeTranslation, so there may be some IA64 BIOS implementations
>> accidentally using TypeStatic. That's why we parsing SparseTranslation
>> flag without checking TranslationType flag. I feel ARM64 may face the
>> same situation as IA64:(
>>
>> We may expect (TypeStatic, 0-offset) and (TypeTranslation,
>> non-0-offset) in real word. For other two combinations, I haven't
>> found a real usage yet, though theoretically they are possible.
>>
>
> I think we should not bend the generic code for IA64 only and expose
> other platforms to the same issue. Instead, lets interpret spec
> correctly and create IA64 quirk for the sake of backward compatibility.
> Thoughts?
I think there are at least two factors related to this issue.
First we still lack of a way/framework to fix errors in ACPI resource
descriptors. Recently we have refined ACPI resource parsing interfaces
and enforced strictly sanity check. This brings us some regressions
which are really BIOS flaws, but it used to work and now breaks:(
I'm still struggling to get those regressions fixed. So we may run
into the same situation if we enforce strict check for TranslationType:(
Second enforcing strict check doesn't bring us too much benifits.
Translation type is almost platform specific, and we haven't found a
platform support both TypeTranslation and TypeStatic, so arch code
may assume the correct translation type no matter what BIOS reports.
So it won't hurt us even BIOS reports wrong translation type.
So I'm tending to keep current implementation with looser checking,
otherwise it may cause regressions.
Thanks,
Gerry
On 12.11.2015 16:05, Jiang Liu wrote:
> On 2015/11/12 22:45, Tomasz Nowicki wrote:
>> On 12.11.2015 15:04, Jiang Liu wrote:
>>> On 2015/11/12 21:21, Tomasz Nowicki wrote:
>>>> On 12.11.2015 09:43, Jiang Liu wrote:
>>>>> On 2015/11/12 1:46, Lorenzo Pieralisi wrote:
>>>>>> On Tue, Nov 10, 2015 at 01:50:46PM +0800, Jiang Liu wrote:
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>>> In particular, I would like to understand, for an eg DWordIO
>>>>>>>>> descriptor,
>>>>>>>>> what Range Minimum, Range Maximum and Translation Offset represent,
>>>>>>>>> they can't mean different things depending on the SW parsing them,
>>>>>>>>> this totally defeats the purpose.
>>>>>>>>
>>>>>>>> I have no clue about what those mean in ACPI though.
>>>>>>>>
>>>>>>>> Generally speaking, each PCI domain is expected to have a (normally
>>>>>>>> 64KB)
>>>>>>>> range of CPU addresses that gets translated into PCI I/O space the
>>>>>>>> same
>>>>>>>> way that config space and memory space are handled.
>>>>>>>> This is true for almost every architecture except for x86, which
>>>>>>>> uses
>>>>>>>> different CPU instructions for I/O space compared to the other
>>>>>>>> spaces.
>>>>>>>>
>>>>>>>>> By the way, ia64 ioremaps the translation_offset (ie
>>>>>>>>> new_space()), so
>>>>>>>>> basically that's the CPU physical address at which the PCI host
>>>>>>>>> bridge
>>>>>>>>> map the IO space transactions), I do not think ia64 is any
>>>>>>>>> different from
>>>>>>>>> arm64 in this respect, if it is please provide an HW description
>>>>>>>>> here from
>>>>>>>>> the PCI bus perspective here (also an example of ia64 ACPI PCI
>>>>>>>>> host bridge
>>>>>>>>> tables would help).
>>>>>>>>
>>>>>>>> The main difference between ia64 and a lot of the other
>>>>>>>> architectures (e.g.
>>>>>>>> sparc is different again) is that ia64 defines a logical address
>>>>>>>> range
>>>>>>>> in terms of having a small number for each I/O space followed by the
>>>>>>>> offset within that space as a 'port number' and uses a mapping
>>>>>>>> function
>>>>>>>> that is defined as
>>>>>>>>
>>>>>>>> static inline void *__ia64_mk_io_addr (unsigned long port)
>>>>>>>> {
>>>>>>>> struct io_space *space = &io_space[IO_SPACE_NR(port)];
>>>>>>>> return (space->mmio_base | IO_SPACE_PORT(port););
>>>>>>>> }
>>>>>>>> static inline unsigned int inl(unsigned long port)
>>>>>>>> {
>>>>>>>> return *__ia64_mk_io_addr(port);
>>>>>>>> }
>>>>>>>>
>>>>>>>> Most architectures allow only one I/O port range and put it at a
>>>>>>>> fixed
>>>>>>>> virtual address so that inl() simply becomes
>>>>>>>>
>>>>>>>> static inline u32 inl(unsigned long addr)
>>>>>>>> {
>>>>>>>> return readl(PCI_IOBASE + addr);
>>>>>>>> }
>>>>>>>>
>>>>>>>> which noticeably reduces code size.
>>>>>>>>
>>>>>>>> On some architectures (powerpc, arm, arm64), we then get the same
>>>>>>>> simplified
>>>>>>>> definition with a fixed virtual address, and use pci_ioremap_io() or
>>>>>>>> something like that to to map a physical address range into this
>>>>>>>> virtual
>>>>>>>> address window at the correct io_offset;
>>>>>>> Hi all,
>>>>>>> Thanks for explanation, I found a way to make the ACPI resource
>>>>>>> parsing interface arch neutral, it should help to address Lorenzo's
>>>>>>> concern. Please refer to the attached patch. (It's still RFC, not
>>>>>>> tested
>>>>>>> yet).
>>>>>>
>>>>>> If we go with this approach though, you are not adding the offset to
>>>>>> the resource when parsing the memory spaces in acpi_decode_space(),
>>>>>> are we
>>>>>> sure that's what we really want ?
>>>>>>
>>>>>> In DT, a host bridge range has a:
>>>>>>
>>>>>> - CPU physical address
>>>>>> - PCI bus address
>>>>>>
>>>>>> We use that to compute the offset between primary bus (ie CPU physical
>>>>>> address) and secondary bus (ie PCI bus address).
>>>>>>
>>>>>> The value ending up in the PCI resource struct (for memory space) is
>>>>>> the CPU physical address, if you do not add the offset in
>>>>>> acpi_decode_space
>>>>>> that does not hold true on platforms where CPU<->PCI offset != 0 on
>>>>>> ACPI,
>>>>>> am I wrong ?
>>>>> Hi Lorenzo,
>>>>> I may have found the divergence between us about the design
>>>>> here. You
>>>>> treat it as a one-stage translation but I treat it as a
>>>>> two-stage translation as below:
>>>>> stage 1: map(translate) per-PCI-domain IO port address[0, 16M) into
>>>>> system global IO port address. Here system global IO port address is
>>>>> ioport_resource[0, IO_SPACE_LIMIT).
>>>>> stage 2: map system IO port address into system memory address.
>>>>>
>>>>> We need two objects of struct resource_win to support above two-stage
>>>>> translation. One object, type of IORESOURCE_IO, is used to support
>>>>> stage one, and it will also used to allocate IO port resources
>>>>> for PCI devices. Another object, type of IORESOURCE_MMIO, is used
>>>>> to allocate resource from iomem_resource and setup MMIO mapping
>>>>> to actually access IO ports.
>>>>>
>>>>> For ARM64, it doesn't support multiple per-PCI-domain(bus local)
>>>>> IO port address space yet, so stage one seems to be optional
>>>>> becomes the offset between bus local IO port address and system
>>>>> IO port address is always 0. But we still need two objects of
>>>>> struct resource_win. The first object is
>>>>> {
>>>>> offset:0,
>>>>> start:AddressMinimum,
>>>>> end:AddressMaximum,
>>>>> flags:IORESOURCE_IO
>>>>> }
>>>>> Here it's type of IORESOURCE_IO and offset must be zero because
>>>>> pcibios_resource_to_bus() will access it translate system IO
>>>>> port address into bus local IO port address. With my patch,
>>>>> the struct resource_win object created by the ACPI core will
>>>>> be reused for this.
>>>>>
>>>>> The second object is:
>>>>> {
>>>>> offset:Translation_Offset,
>>>>> start:AddressMinimum + Translation_Offset,
>>>>> end:AddressMaximum + Translation_Offset,
>>>>> flags:IORESOURCE_MMIO
>>>>> }
>>>>> Arch code need to create the second struct resource_win object
>>>>> and actually setup the MMIO mapping.
>>>>>
>>>>> But there's really another bug need to get fixed, funciton
>>>>> acpi_dev_ioresource_flags() assumes bus local IO port address
>>>>> space is size of 64K, which is wrong for IA64 and ARM64.
>>>>>
>>>>
>>>> So what would be the Translation_Offset meaning for two cases DWordIo
>>>> (....,TypeTranslation) vs DWordIo (....,TypeStatic)? And why we did not
>>>> use TypeTranslation for IA64 so far?
>>>
>>> IA64 actually ignores the translation type flag and just assume it's
>>> TypeTranslation, so there may be some IA64 BIOS implementations
>>> accidentally using TypeStatic. That's why we parsing SparseTranslation
>>> flag without checking TranslationType flag. I feel ARM64 may face the
>>> same situation as IA64:(
>>>
>>> We may expect (TypeStatic, 0-offset) and (TypeTranslation,
>>> non-0-offset) in real word. For other two combinations, I haven't
>>> found a real usage yet, though theoretically they are possible.
>>>
>>
>> I think we should not bend the generic code for IA64 only and expose
>> other platforms to the same issue. Instead, lets interpret spec
>> correctly and create IA64 quirk for the sake of backward compatibility.
>> Thoughts?
> I think there are at least two factors related to this issue.
>
> First we still lack of a way/framework to fix errors in ACPI resource
> descriptors. Recently we have refined ACPI resource parsing interfaces
> and enforced strictly sanity check. This brings us some regressions
> which are really BIOS flaws, but it used to work and now breaks:(
> I'm still struggling to get those regressions fixed. So we may run
> into the same situation if we enforce strict check for TranslationType:(
>
> Second enforcing strict check doesn't bring us too much benifits.
> Translation type is almost platform specific, and we haven't found a
> platform support both TypeTranslation and TypeStatic, so arch code
> may assume the correct translation type no matter what BIOS reports.
> So it won't hurt us even BIOS reports wrong translation type.
>
That is my point, lets pass down all we need from resource range
descriptors to arch code, then archs with known quirks can whatever is
needed to make it works. However, generic code like acpi_decode_space
cannot play with offsets with silent IA64 assumption.
To sum it up, your last patch looks ok to me modulo Lorenzo's concern:
>>>>>> If we go with this approach though, you are not adding the offset to
>>>>>> the resource when parsing the memory spaces in acpi_decode_space(),
>>>>>> are we
>>>>>> sure that's what we really want ?
>>>>>>
>>>>>> In DT, a host bridge range has a:
>>>>>>
>>>>>> - CPU physical address
>>>>>> - PCI bus address
>>>>>>
>>>>>> We use that to compute the offset between primary bus (ie CPU
physical
>>>>>> address) and secondary bus (ie PCI bus address).
>>>>>>
>>>>>> The value ending up in the PCI resource struct (for memory space) is
>>>>>> the CPU physical address, if you do not add the offset in
>>>>>> acpi_decode_space
>>>>>> that does not hold true on platforms where CPU<->PCI offset != 0 on
>>>>>> ACPI,
>>>>>> am I wrong ?
His concern is that your patch will cause:
acpi_pci_root_validate_resources(&device->dev, list,
IORESOURCE_MEM);
to fail now.
Regards,
Tomasz
Please trim your emails, thanks.
On Fri, Nov 13, 2015 at 01:57:30PM +0100, Tomasz Nowicki wrote:
> On 12.11.2015 16:05, Jiang Liu wrote:
[...]
> >>>IA64 actually ignores the translation type flag and just assume it's
> >>>TypeTranslation, so there may be some IA64 BIOS implementations
> >>>accidentally using TypeStatic. That's why we parsing SparseTranslation
> >>>flag without checking TranslationType flag. I feel ARM64 may face the
> >>>same situation as IA64:(
> >>>
> >>>We may expect (TypeStatic, 0-offset) and (TypeTranslation,
> >>>non-0-offset) in real word. For other two combinations, I haven't
> >>>found a real usage yet, though theoretically they are possible.
I do not understand why (TypeStatic, non-0-offset) is not a valid
option. Aren't there any (x86) platforms with a CPU<->PCI _physical_
address space offset out there (I am talking about memory space) ?
> >>I think we should not bend the generic code for IA64 only and expose
> >>other platforms to the same issue. Instead, lets interpret spec
> >>correctly and create IA64 quirk for the sake of backward compatibility.
> >>Thoughts?
> >I think there are at least two factors related to this issue.
> >
> >First we still lack of a way/framework to fix errors in ACPI resource
> >descriptors. Recently we have refined ACPI resource parsing interfaces
> >and enforced strictly sanity check. This brings us some regressions
> >which are really BIOS flaws, but it used to work and now breaks:(
> >I'm still struggling to get those regressions fixed. So we may run
> >into the same situation if we enforce strict check for TranslationType:(
> >
> >Second enforcing strict check doesn't bring us too much benifits.
> >Translation type is almost platform specific, and we haven't found a
> >platform support both TypeTranslation and TypeStatic, so arch code
> >may assume the correct translation type no matter what BIOS reports.
> >So it won't hurt us even BIOS reports wrong translation type.
TBH I still do not understand what TranslationType actually means,
I will ask whoever added that to the specification to understand it.
> That is my point, lets pass down all we need from resource range
> descriptors to arch code, then archs with known quirks can whatever
> is needed to make it works. However, generic code like
> acpi_decode_space cannot play with offsets with silent IA64
> assumption.
>
> To sum it up, your last patch looks ok to me modulo Lorenzo's concern:
> >>>>>> If we go with this approach though, you are not adding the offset to
> >>>>>> the resource when parsing the memory spaces in acpi_decode_space(),
> >>>>>> are we
> >>>>>> sure that's what we really want ?
> >>>>>>
> >>>>>> In DT, a host bridge range has a:
> >>>>>>
> >>>>>> - CPU physical address
> >>>>>> - PCI bus address
> >>>>>>
> >>>>>> We use that to compute the offset between primary bus (ie CPU
> physical
> >>>>>> address) and secondary bus (ie PCI bus address).
> >>>>>>
> >>>>>> The value ending up in the PCI resource struct (for memory space) is
> >>>>>> the CPU physical address, if you do not add the offset in
> >>>>>> acpi_decode_space
> >>>>>> that does not hold true on platforms where CPU<->PCI offset != 0 on
> >>>>>> ACPI,
> >>>>>> am I wrong ?
> His concern is that your patch will cause:
> acpi_pci_root_validate_resources(&device->dev, list,
> IORESOURCE_MEM);
> to fail now.
Not really. My concern is that there might be platforms out there with
an offset between the CPU and PCI physical address spaces, and if we
remove the offset value in acpi_decode_space we can break them,
because in the kernel struct resource data we have to have CPU physical
addresses, not PCI ones. If offset == 0, we are home and dry, I do not
understand why that's a given, which is what we would assume if Jiang's
patch is merged as-is unless I am mistaken.
Thanks,
Lorenzo
On 2015/11/14 1:03, Lorenzo Pieralisi wrote:
> Please trim your emails, thanks.
>
> On Fri, Nov 13, 2015 at 01:57:30PM +0100, Tomasz Nowicki wrote:
>> On 12.11.2015 16:05, Jiang Liu wrote:
>
> [...]
>
>>>>> IA64 actually ignores the translation type flag and just assume it's
>>>>> TypeTranslation, so there may be some IA64 BIOS implementations
>>>>> accidentally using TypeStatic. That's why we parsing SparseTranslation
>>>>> flag without checking TranslationType flag. I feel ARM64 may face the
>>>>> same situation as IA64:(
>>>>>
>>>>> We may expect (TypeStatic, 0-offset) and (TypeTranslation,
>>>>> non-0-offset) in real word. For other two combinations, I haven't
>>>>> found a real usage yet, though theoretically they are possible.
>
> I do not understand why (TypeStatic, non-0-offset) is not a valid
> option. Aren't there any (x86) platforms with a CPU<->PCI _physical_
> address space offset out there (I am talking about memory space) ?
It's possible, but we have found such a design yet. If we eventually
encounter such a case, we need to enhance x86 specific code to support
it.
>
>>>> I think we should not bend the generic code for IA64 only and expose
>>>> other platforms to the same issue. Instead, lets interpret spec
>>>> correctly and create IA64 quirk for the sake of backward compatibility.
>>>> Thoughts?
>>> I think there are at least two factors related to this issue.
>>>
>>> First we still lack of a way/framework to fix errors in ACPI resource
>>> descriptors. Recently we have refined ACPI resource parsing interfaces
>>> and enforced strictly sanity check. This brings us some regressions
>>> which are really BIOS flaws, but it used to work and now breaks:(
>>> I'm still struggling to get those regressions fixed. So we may run
>>> into the same situation if we enforce strict check for TranslationType:(
>>>
>>> Second enforcing strict check doesn't bring us too much benifits.
>>> Translation type is almost platform specific, and we haven't found a
>>> platform support both TypeTranslation and TypeStatic, so arch code
>>> may assume the correct translation type no matter what BIOS reports.
>>> So it won't hurt us even BIOS reports wrong translation type.
>
> TBH I still do not understand what TranslationType actually means,
> I will ask whoever added that to the specification to understand it.
>
>> That is my point, lets pass down all we need from resource range
>> descriptors to arch code, then archs with known quirks can whatever
>> is needed to make it works. However, generic code like
>> acpi_decode_space cannot play with offsets with silent IA64
>> assumption.
>>
>> To sum it up, your last patch looks ok to me modulo Lorenzo's concern:
>>>>>>>> If we go with this approach though, you are not adding the offset to
>>>>>>>> the resource when parsing the memory spaces in acpi_decode_space(),
>>>>>>>> are we
>>>>>>>> sure that's what we really want ?
>>>>>>>>
>>>>>>>> In DT, a host bridge range has a:
>>>>>>>>
>>>>>>>> - CPU physical address
>>>>>>>> - PCI bus address
>>>>>>>>
>>>>>>>> We use that to compute the offset between primary bus (ie CPU
>> physical
>>>>>>>> address) and secondary bus (ie PCI bus address).
>>>>>>>>
>>>>>>>> The value ending up in the PCI resource struct (for memory space) is
>>>>>>>> the CPU physical address, if you do not add the offset in
>>>>>>>> acpi_decode_space
>>>>>>>> that does not hold true on platforms where CPU<->PCI offset != 0 on
>>>>>>>> ACPI,
>>>>>>>> am I wrong ?
>> His concern is that your patch will cause:
>> acpi_pci_root_validate_resources(&device->dev, list,
>> IORESOURCE_MEM);
>> to fail now.
>
> Not really. My concern is that there might be platforms out there with
> an offset between the CPU and PCI physical address spaces, and if we
> remove the offset value in acpi_decode_space we can break them,
> because in the kernel struct resource data we have to have CPU physical
> addresses, not PCI ones. If offset == 0, we are home and dry, I do not
> understand why that's a given, which is what we would assume if Jiang's
> patch is merged as-is unless I am mistaken.
We try to exclude offset from struct resource in generic ACPI code,
and it's the arch's responsibility to decide how to manipulate struct
resource object if offset is not zero.
Currently offset is always zero for x86, and IA64 has arch specific
code to handle non-zero offset. So we should be safe without breaking
existing code. For ARM64, it's a little different from IA64 so it's
hard to share code between IA64 and ARM64.
>
> Thanks,
> Lorenzo
>
Hi Jiang,
On Sat, Nov 14, 2015 at 01:49:08AM +0800, Jiang Liu wrote:
[...]
> > Not really. My concern is that there might be platforms out there with
> > an offset between the CPU and PCI physical address spaces, and if we
> > remove the offset value in acpi_decode_space we can break them,
> > because in the kernel struct resource data we have to have CPU physical
> > addresses, not PCI ones. If offset == 0, we are home and dry, I do not
> > understand why that's a given, which is what we would assume if Jiang's
> > patch is merged as-is unless I am mistaken.
> We try to exclude offset from struct resource in generic ACPI code,
> and it's the arch's responsibility to decide how to manipulate struct
> resource object if offset is not zero.
>
> Currently offset is always zero for x86, and IA64 has arch specific
> code to handle non-zero offset. So we should be safe without breaking
> existing code. For ARM64, it's a little different from IA64 so it's
> hard to share code between IA64 and ARM64.
Can you drop the patch on the mailing lists please, we actually need it
to get ACPI ARM64 PCIe support in the kernel, please let me know how
you want to handle this and if you need any help.
Thanks,
Lorenzo
On 11/6/2015 10:44 AM, Jiang Liu wrote:
> On 2015/11/6 23:32, Jiang Liu wrote:
>> On 2015/11/6 22:45, Lorenzo Pieralisi wrote:
>>> On Fri, Nov 06, 2015 at 09:22:46PM +0800, Jiang Liu wrote:
>>>> On 2015/11/6 20:40, Tomasz Nowicki wrote:
>>>>> 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:
>>>>>>> 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.
>>>> How about the attached to patch to support TypeTranslation?
>>>> It only passes compilation:)
>>>
>>> Eh, hopefully there are not any ACPI tables out there with that bit
>>> set that work _today_ and would not work with the patch attached :)
>>>
>>> My question is still there: do we want to handle the same problem
>>> as ia64 has in a different manner ? Certainly we won't be able
>>> to update ia64 platforms ACPI tables, so we would end up with
>>> two platforms handling IO resources in different ways unless I am
>>> missing something here.
>> There are some difference between IA64 and ARM64.
>> On IA64, it supports 16M IO address space per PCI domain and 256 PCI
>> domains at max. So the system IO address space is 16M * 256 = 4G.
>> So it does two level translations to support IO port
>> 1) translate PCI bus local IO port address into system global IO port
>> address by adding acpi_des->translation_offset.
>> 2) translate the 4G system IO port address space into MMIO address.
>> IA64 has reserved a 4G space for IO port mapping. This translation
>> is done by arch specific method.
>> In other word, IA64 needs two level translation, but ACPI only provides
>> on (trans_type, trans_offset) pair for encoding, so it's used for step 1).
>>
>> For ARM64, I think currently it only needs step 2).
>>
>>>
>>> BTW, why would we add offset to res->start only if TypeTranslation is
>>> clear ? Is not that something we would do just to make things "work" ?
>>> That flag has no bearing on the offset, only on the resource type AFAIK.
>> It's not a hack, but a way to interpret ACPI spec:)
>>
>> With current linux resource management framework, we need to allocate
>> both MMIO and IO port address space range for an ACPI resource of type
>> 'TypeTranslation'. And struct resource could be either IO port or MMIO,
>> not both. So the choice is to keep the resource as IO port, and let
>> arch code to build the special MMIO mapping for it. Otherwise it will
>> break too many things if we convert the resource as MMIO.
>>
>> That said, we need to add translation_offset to convert bus local
>> IO port address into system global IO port address if it's type of
>> TypeStatic, because ioresource_ioport uses system global IO port
>> address.
>>
>> For an ACPI resource of type TypeTranslation, system global IO port
>> address equals bus local IO port address, and the translation_offset
>> is used to translate IO port address into MMIO address, so we shouldn't
>> add translation_offset to the IO port resource descriptor.
> One note for the TypeTranslation case, the arch code needs to reset
> resource_win->offset to zero after setting up the MMIO map. Sample
> code as below:
> va = ioremap(resource_win->offset + res->start, resource_size(res));
> resource_win->offset = 0;
>
> Otherwise it will break pcibios_resource_to_bus() etc.
>
>>
>> Thanks,
>> Gerry
>>
>>>
>>> This without taking into account ARM64 systems shipping with ACPI
>>> tables that does not set the TypeTranslation at present.
>>>
>>> On top of that, I noticed that core ACPI code handles Sparse
>>> Translation (ie _TRS), that should be considered meaningful only if _TTP
>>> is set (and that's not checked).
>> Yes, that's a flaw:(
>>
>>>
>>> Thoughts ?
>>>
>>> Thanks,
>>> Lorenzo
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Tomasz, Lorenzo;
I have an ARMv8 platform with IO address support where I can test this
implementation. Let me know if there is any patch that I can try.
Sinan
--
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
On 20.11.2015 11:18, Lorenzo Pieralisi wrote:
> Hi Jiang,
>
> On Sat, Nov 14, 2015 at 01:49:08AM +0800, Jiang Liu wrote:
>
> [...]
>
>>> Not really. My concern is that there might be platforms out there with
>>> an offset between the CPU and PCI physical address spaces, and if we
>>> remove the offset value in acpi_decode_space we can break them,
>>> because in the kernel struct resource data we have to have CPU physical
>>> addresses, not PCI ones. If offset == 0, we are home and dry, I do not
>>> understand why that's a given, which is what we would assume if Jiang's
>>> patch is merged as-is unless I am mistaken.
>> We try to exclude offset from struct resource in generic ACPI code,
>> and it's the arch's responsibility to decide how to manipulate struct
>> resource object if offset is not zero.
>>
>> Currently offset is always zero for x86, and IA64 has arch specific
>> code to handle non-zero offset. So we should be safe without breaking
>> existing code. For ARM64, it's a little different from IA64 so it's
>> hard to share code between IA64 and ARM64.
>
> Can you drop the patch on the mailing lists please, we actually need it
> to get ACPI ARM64 PCIe support in the kernel, please let me know how
> you want to handle this and if you need any help.
>
Kindly reminder.
Regards,
Tomasz