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

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 <lorenzo.pieralisi@arm.com>
Tested-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
Signed-off-by: Liu Jiang <jiang.liu@linux.intel.com>
---
 drivers/acpi/pci_root.c  |  204 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-acpi.h |   24 ++++++
 2 files changed, 228 insertions(+)

On Wed, Oct 14, 2015 at 02:29:39PM +0800, 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 <lorenzo.pieralisi@arm.com>
> Tested-by: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> Signed-off-by: Liu Jiang <jiang.liu@linux.intel.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  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);
> +	}
> +
> +	return ret;
> +}
> +
> +static void pci_acpi_root_add_resources(struct acpi_pci_root_info *info)
> +{
> +	struct resource_entry *entry, *tmp;
> +	struct resource *res, *conflict, *root = NULL;
> +
> +	resource_list_for_each_entry_safe(entry, tmp, &info->resources) {
> +		res = entry->res;
> +		if (res->flags & IORESOURCE_MEM)
> +			root = &iomem_resource;
> +		else if (res->flags & IORESOURCE_IO)
> +			root = &ioport_resource;
> +		else
> +			continue;
> +
> +		conflict = insert_resource_conflict(root, res);
> +		if (conflict) {
> +			dev_info(&info->bridge->dev,
> +				 "ignoring host bridge window %pR (conflicts with %s %pR)\n",
> +				 res, conflict->name, conflict);
> +			resource_list_destroy_entry(entry);
> +		}
> +	}
> +}
> +
> +static void __acpi_pci_root_release_info(struct acpi_pci_root_info *info)
> +{
> +	struct resource *res;
> +	struct resource_entry *entry, *tmp;
> +
> +	if (!info)
> +		return;
> +
> +	resource_list_for_each_entry_safe(entry, tmp, &info->resources) {
> +		res = entry->res;
> +		if (res->parent &&
> +		    (res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
> +			release_resource(res);
> +		resource_list_destroy_entry(entry);
> +	}
> +
> +	info->ops->release_info(info);
> +}
> +
> +static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
> +{
> +	struct resource *res;
> +	struct resource_entry *entry;
> +
> +	resource_list_for_each_entry(entry, &bridge->windows) {
> +		res = entry->res;
> +		if (res->parent &&
> +		    (res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
> +			release_resource(res);
> +	}
> +	__acpi_pci_root_release_info(bridge->release_data);
> +}
> +
> +struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> +				     struct acpi_pci_root_ops *ops,
> +				     struct acpi_pci_root_info *info,
> +				     void *sysdata)
> +{
> +	int ret, busnum = root->secondary.start;
> +	struct acpi_device *device = root->device;
> +	int node = acpi_get_node(device->handle);
> +	struct pci_bus *bus;
> +
> +	info->root = root;
> +	info->bridge = device;
> +	info->ops = ops;
> +	INIT_LIST_HEAD(&info->resources);
> +	snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
> +		 root->segment, busnum);
> +
> +	if (ops->init_info && ops->init_info(info))
> +		goto out_release_info;
> +	if (ops->prepare_resources)
> +		ret = ops->prepare_resources(info);
> +	else
> +		ret = acpi_pci_probe_root_resources(info);
> +	if (ret < 0)
> +		goto out_release_info;
> +
> +	pci_acpi_root_add_resources(info);
> +	pci_add_resource(&info->resources, &root->secondary);
> +	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
> +				  sysdata, &info->resources);
> +	if (!bus)
> +		goto out_release_info;
> +
> +	pci_scan_child_bus(bus);
> +	pci_set_host_bridge_release(to_pci_host_bridge(bus->bridge),
> +				    acpi_pci_root_release_info, info);
> +	if (node != NUMA_NO_NODE)
> +		dev_printk(KERN_DEBUG, &bus->dev, "on NUMA node %d\n", node);
> +	return bus;
> +
> +out_release_info:
> +	__acpi_pci_root_release_info(info);
> +	return NULL;
> +}
> +
>  void __init acpi_pci_root_init(void)
>  {
>  	acpi_hest_init();
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index a965efa52152..89ab0572dbc6 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -52,6 +52,30 @@ static inline acpi_handle acpi_pci_get_bridge_handle(struct pci_bus *pbus)
>  	return ACPI_HANDLE(dev);
>  }
>  
> +struct acpi_pci_root;
> +struct acpi_pci_root_ops;
> +
> +struct acpi_pci_root_info {
> +	struct acpi_pci_root		*root;
> +	struct acpi_device		*bridge;
> +	struct acpi_pci_root_ops	*ops;
> +	struct list_head		resources;
> +	char				name[16];
> +};
> +
> +struct acpi_pci_root_ops {
> +	struct pci_ops *pci_ops;
> +	int (*init_info)(struct acpi_pci_root_info *info);
> +	void (*release_info)(struct acpi_pci_root_info *info);
> +	int (*prepare_resources)(struct acpi_pci_root_info *info);
> +};
> +
> +extern int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info);
> +extern struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> +					    struct acpi_pci_root_ops *ops,
> +					    struct acpi_pci_root_info *info,
> +					    void *sd);
> +
>  void acpi_pci_add_bus(struct pci_bus *bus);
>  void acpi_pci_remove_bus(struct pci_bus *bus);
>  
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 <lorenzo.pieralisi@arm.com>
> Tested-by: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> Signed-off-by: Liu Jiang <jiang.liu@linux.intel.com>
> ---
>   drivers/acpi/pci_root.c  |  204 ++++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/pci-acpi.h |   24 ++++++
>   2 files changed, 228 insertions(+)
>

[...]

> +
> +struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> +				     struct acpi_pci_root_ops *ops,
> +				     struct acpi_pci_root_info *info,
> +				     void *sysdata)
> +{
> +	int ret, busnum = root->secondary.start;
> +	struct acpi_device *device = root->device;
> +	int node = acpi_get_node(device->handle);
> +	struct pci_bus *bus;
> +
> +	info->root = root;
> +	info->bridge = device;
> +	info->ops = ops;
> +	INIT_LIST_HEAD(&info->resources);
> +	snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
> +		 root->segment, busnum);
> +
> +	if (ops->init_info && ops->init_info(info))
> +		goto out_release_info;
> +	if (ops->prepare_resources)
> +		ret = ops->prepare_resources(info);
> +	else
> +		ret = acpi_pci_probe_root_resources(info);
> +	if (ret < 0)
> +		goto out_release_info;
> +
> +	pci_acpi_root_add_resources(info);
> +	pci_add_resource(&info->resources, &root->secondary);
> +	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
> +				  sysdata, &info->resources);

Thank a lot for this cleanup!!

I recall you already considered passing segment (domain nr) to 
pci_create_root_bus, right? Can you please remind me why we gave up on this?

I am asking because currently I can not find the way to retrieve domain 
number from pci_bus_assign_domain_nr (for those platforms which choose 
PCI_DOMAINS_GENERIC and want to use segment nr from MCFG table) which is 
the part of pci_create_root_bus.

Regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Oct 21, 2015 at 11:57:53AM +0200, 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 <lorenzo.pieralisi@arm.com>
> >Tested-by: Tony Luck <tony.luck@intel.com>
> >Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> >Signed-off-by: Liu Jiang <jiang.liu@linux.intel.com>
> >---
> >  drivers/acpi/pci_root.c  |  204 ++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/pci-acpi.h |   24 ++++++
> >  2 files changed, 228 insertions(+)
> >
> 
> [...]
> 
> >+
> >+struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> >+				     struct acpi_pci_root_ops *ops,
> >+				     struct acpi_pci_root_info *info,
> >+				     void *sysdata)
> >+{
> >+	int ret, busnum = root->secondary.start;
> >+	struct acpi_device *device = root->device;
> >+	int node = acpi_get_node(device->handle);
> >+	struct pci_bus *bus;
> >+
> >+	info->root = root;
> >+	info->bridge = device;
> >+	info->ops = ops;
> >+	INIT_LIST_HEAD(&info->resources);
> >+	snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
> >+		 root->segment, busnum);
> >+
> >+	if (ops->init_info && ops->init_info(info))
> >+		goto out_release_info;
> >+	if (ops->prepare_resources)
> >+		ret = ops->prepare_resources(info);
> >+	else
> >+		ret = acpi_pci_probe_root_resources(info);
> >+	if (ret < 0)
> >+		goto out_release_info;
> >+
> >+	pci_acpi_root_add_resources(info);
> >+	pci_add_resource(&info->resources, &root->secondary);
> >+	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
> >+				  sysdata, &info->resources);
> 
> Thank a lot for this cleanup!!
> 
> I recall you already considered passing segment (domain nr) to
> pci_create_root_bus, right? Can you please remind me why we gave up on this?
> 
> I am asking because currently I can not find the way to retrieve domain
> number from pci_bus_assign_domain_nr (for those platforms which choose
> PCI_DOMAINS_GENERIC and want to use segment nr from MCFG table) which is the
> part of pci_create_root_bus.

Not sure I fully understand your question, but pci_bus_assign_domain_nr() will
put the assigned domain number in bus->domain_nr if you chose PCI_DOMAINS_GENERIC.
Do you want to override that value with the segment nr from MCFG?

Best regards,
Liviu

> 
> Regards,
> Tomasz
>

On 21.10.2015 13:02, Liviu Dudau wrote:
> On Wed, Oct 21, 2015 at 11:57:53AM +0200, 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 <lorenzo.pieralisi@arm.com>
>>> Tested-by: Tony Luck <tony.luck@intel.com>
>>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
>>> Signed-off-by: Liu Jiang <jiang.liu@linux.intel.com>
>>> ---
>>>   drivers/acpi/pci_root.c  |  204 ++++++++++++++++++++++++++++++++++++++++++++++
>>>   include/linux/pci-acpi.h |   24 ++++++
>>>   2 files changed, 228 insertions(+)
>>>
>>
>> [...]
>>
>>> +
>>> +struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>>> +				     struct acpi_pci_root_ops *ops,
>>> +				     struct acpi_pci_root_info *info,
>>> +				     void *sysdata)
>>> +{
>>> +	int ret, busnum = root->secondary.start;
>>> +	struct acpi_device *device = root->device;
>>> +	int node = acpi_get_node(device->handle);
>>> +	struct pci_bus *bus;
>>> +
>>> +	info->root = root;
>>> +	info->bridge = device;
>>> +	info->ops = ops;
>>> +	INIT_LIST_HEAD(&info->resources);
>>> +	snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
>>> +		 root->segment, busnum);
>>> +
>>> +	if (ops->init_info && ops->init_info(info))
>>> +		goto out_release_info;
>>> +	if (ops->prepare_resources)
>>> +		ret = ops->prepare_resources(info);
>>> +	else
>>> +		ret = acpi_pci_probe_root_resources(info);
>>> +	if (ret < 0)
>>> +		goto out_release_info;
>>> +
>>> +	pci_acpi_root_add_resources(info);
>>> +	pci_add_resource(&info->resources, &root->secondary);
>>> +	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
>>> +				  sysdata, &info->resources);
>>
>> Thank a lot for this cleanup!!
>>
>> I recall you already considered passing segment (domain nr) to
>> pci_create_root_bus, right? Can you please remind me why we gave up on this?
>>
>> I am asking because currently I can not find the way to retrieve domain
>> number from pci_bus_assign_domain_nr (for those platforms which choose
>> PCI_DOMAINS_GENERIC and want to use segment nr from MCFG table) which is the
>> part of pci_create_root_bus.
>
> Not sure I fully understand your question, but pci_bus_assign_domain_nr() will
> put the assigned domain number in bus->domain_nr if you chose PCI_DOMAINS_GENERIC.
> Do you want to override that value with the segment nr from MCFG?
>

Let me give ACPI ARM64 example:

1. We parse MCFG table and get segment nr assigned to root bridge
2. Then PCI host bridge calls acpi_pci_root_create -> 
pci_create_root_bus -> pci_bus_assign_domain_nr
3. At this point we cannot get segment nr for ACPI

So I would like to assign MCFG segment nr to bus->domain_nr being in 
pci_bus_assign_domain_nr giving we have scenario above.

Thanks,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Oct 21, 2015 at 01:27:33PM +0200, Tomasz Nowicki wrote:
> On 21.10.2015 13:02, Liviu Dudau wrote:
> >On Wed, Oct 21, 2015 at 11:57:53AM +0200, 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 <lorenzo.pieralisi@arm.com>
> >>>Tested-by: Tony Luck <tony.luck@intel.com>
> >>>Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> >>>Signed-off-by: Liu Jiang <jiang.liu@linux.intel.com>
> >>>---
> >>>  drivers/acpi/pci_root.c  |  204 ++++++++++++++++++++++++++++++++++++++++++++++
> >>>  include/linux/pci-acpi.h |   24 ++++++
> >>>  2 files changed, 228 insertions(+)
> >>>
> >>
> >>[...]
> >>
> >>>+
> >>>+struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> >>>+				     struct acpi_pci_root_ops *ops,
> >>>+				     struct acpi_pci_root_info *info,
> >>>+				     void *sysdata)
> >>>+{
> >>>+	int ret, busnum = root->secondary.start;
> >>>+	struct acpi_device *device = root->device;
> >>>+	int node = acpi_get_node(device->handle);
> >>>+	struct pci_bus *bus;
> >>>+
> >>>+	info->root = root;
> >>>+	info->bridge = device;
> >>>+	info->ops = ops;
> >>>+	INIT_LIST_HEAD(&info->resources);
> >>>+	snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
> >>>+		 root->segment, busnum);
> >>>+
> >>>+	if (ops->init_info && ops->init_info(info))
> >>>+		goto out_release_info;
> >>>+	if (ops->prepare_resources)
> >>>+		ret = ops->prepare_resources(info);
> >>>+	else
> >>>+		ret = acpi_pci_probe_root_resources(info);
> >>>+	if (ret < 0)
> >>>+		goto out_release_info;
> >>>+
> >>>+	pci_acpi_root_add_resources(info);
> >>>+	pci_add_resource(&info->resources, &root->secondary);
> >>>+	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
> >>>+				  sysdata, &info->resources);
> >>
> >>Thank a lot for this cleanup!!
> >>
> >>I recall you already considered passing segment (domain nr) to
> >>pci_create_root_bus, right? Can you please remind me why we gave up on this?
> >>
> >>I am asking because currently I can not find the way to retrieve domain
> >>number from pci_bus_assign_domain_nr (for those platforms which choose
> >>PCI_DOMAINS_GENERIC and want to use segment nr from MCFG table) which is the
> >>part of pci_create_root_bus.
> >
> >Not sure I fully understand your question, but pci_bus_assign_domain_nr() will
> >put the assigned domain number in bus->domain_nr if you chose PCI_DOMAINS_GENERIC.
> >Do you want to override that value with the segment nr from MCFG?
> >
> 
> Let me give ACPI ARM64 example:
> 
> 1. We parse MCFG table and get segment nr assigned to root bridge
> 2. Then PCI host bridge calls acpi_pci_root_create ->
> pci_create_root_bus -> pci_bus_assign_domain_nr
> 3. At this point we cannot get segment nr for ACPI
> 
> So I would like to assign MCFG segment nr to bus->domain_nr being in
> pci_bus_assign_domain_nr giving we have scenario above.

I do not understand what you mean by "assign MCFG segment" here, please
explain. The MCFG segment group number is used to look-up the configuration
space for a given host bridge, not to assign a domain number to it.

The domain_nr above is the value returned by the _SEG object for the host
bridge device.

Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Oct 21, 2015 at 01:27:33PM +0200, Tomasz Nowicki wrote:
> On 21.10.2015 13:02, Liviu Dudau wrote:
> >On Wed, Oct 21, 2015 at 11:57:53AM +0200, 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 <lorenzo.pieralisi@arm.com>
> >>>Tested-by: Tony Luck <tony.luck@intel.com>
> >>>Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> >>>Signed-off-by: Liu Jiang <jiang.liu@linux.intel.com>
> >>>---
> >>>  drivers/acpi/pci_root.c  |  204 ++++++++++++++++++++++++++++++++++++++++++++++
> >>>  include/linux/pci-acpi.h |   24 ++++++
> >>>  2 files changed, 228 insertions(+)
> >>>
> >>
> >>[...]
> >>
> >>>+
> >>>+struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> >>>+				     struct acpi_pci_root_ops *ops,
> >>>+				     struct acpi_pci_root_info *info,
> >>>+				     void *sysdata)
> >>>+{
> >>>+	int ret, busnum = root->secondary.start;
> >>>+	struct acpi_device *device = root->device;
> >>>+	int node = acpi_get_node(device->handle);
> >>>+	struct pci_bus *bus;
> >>>+
> >>>+	info->root = root;
> >>>+	info->bridge = device;
> >>>+	info->ops = ops;
> >>>+	INIT_LIST_HEAD(&info->resources);
> >>>+	snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
> >>>+		 root->segment, busnum);
> >>>+
> >>>+	if (ops->init_info && ops->init_info(info))
> >>>+		goto out_release_info;
> >>>+	if (ops->prepare_resources)
> >>>+		ret = ops->prepare_resources(info);
> >>>+	else
> >>>+		ret = acpi_pci_probe_root_resources(info);
> >>>+	if (ret < 0)
> >>>+		goto out_release_info;
> >>>+
> >>>+	pci_acpi_root_add_resources(info);
> >>>+	pci_add_resource(&info->resources, &root->secondary);
> >>>+	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
> >>>+				  sysdata, &info->resources);
> >>
> >>Thank a lot for this cleanup!!
> >>
> >>I recall you already considered passing segment (domain nr) to
> >>pci_create_root_bus, right? Can you please remind me why we gave up on this?
> >>
> >>I am asking because currently I can not find the way to retrieve domain
> >>number from pci_bus_assign_domain_nr (for those platforms which choose
> >>PCI_DOMAINS_GENERIC and want to use segment nr from MCFG table) which is the
> >>part of pci_create_root_bus.
> >
> >Not sure I fully understand your question, but pci_bus_assign_domain_nr() will
> >put the assigned domain number in bus->domain_nr if you chose PCI_DOMAINS_GENERIC.
> >Do you want to override that value with the segment nr from MCFG?
> >
> 
> Let me give ACPI ARM64 example:
> 
> 1. We parse MCFG table and get segment nr assigned to root bridge
> 2. Then PCI host bridge calls acpi_pci_root_create -> pci_create_root_bus ->
> pci_bus_assign_domain_nr
> 3. At this point we cannot get segment nr for ACPI
> 
> So I would like to assign MCFG segment nr to bus->domain_nr being in
> pci_bus_assign_domain_nr giving we have scenario above.

I thought so. What about adding code to pci_bus_assign_domain_nr() to get the
segment nr from MCFG table? I know the function looks scary and the comments
don't seem to acknowledge that use_dt_domain static variable has actually 3
possible values (-1, 0, 1) but you can use -1 to mean "try ACPI segment nr
first" (just a suggestion, you or others might have a better idea).

Best regards,
Liviu

> 
> Thanks,
> Tomasz
>

On 2015/10/21 19:27, Tomasz Nowicki wrote:
> On 21.10.2015 13:02, Liviu Dudau wrote:
>> On Wed, Oct 21, 2015 at 11:57:53AM +0200, 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 <lorenzo.pieralisi@arm.com>
>>>> Tested-by: Tony Luck <tony.luck@intel.com>
>>>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
>>>> Signed-off-by: Liu Jiang <jiang.liu@linux.intel.com>
>>>> ---
>>>>   drivers/acpi/pci_root.c  |  204
>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>   include/linux/pci-acpi.h |   24 ++++++
>>>>   2 files changed, 228 insertions(+)
>>>>
>>>
>>> [...]
>>>
>>>> +
>>>> +struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>>>> +                     struct acpi_pci_root_ops *ops,
>>>> +                     struct acpi_pci_root_info *info,
>>>> +                     void *sysdata)
>>>> +{
>>>> +    int ret, busnum = root->secondary.start;
>>>> +    struct acpi_device *device = root->device;
>>>> +    int node = acpi_get_node(device->handle);
>>>> +    struct pci_bus *bus;
>>>> +
>>>> +    info->root = root;
>>>> +    info->bridge = device;
>>>> +    info->ops = ops;
>>>> +    INIT_LIST_HEAD(&info->resources);
>>>> +    snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
>>>> +         root->segment, busnum);
>>>> +
>>>> +    if (ops->init_info && ops->init_info(info))
>>>> +        goto out_release_info;
>>>> +    if (ops->prepare_resources)
>>>> +        ret = ops->prepare_resources(info);
>>>> +    else
>>>> +        ret = acpi_pci_probe_root_resources(info);
>>>> +    if (ret < 0)
>>>> +        goto out_release_info;
>>>> +
>>>> +    pci_acpi_root_add_resources(info);
>>>> +    pci_add_resource(&info->resources, &root->secondary);
>>>> +    bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
>>>> +                  sysdata, &info->resources);
>>>
>>> Thank a lot for this cleanup!!
>>>
>>> I recall you already considered passing segment (domain nr) to
>>> pci_create_root_bus, right? Can you please remind me why we gave up
>>> on this?
>>>
>>> I am asking because currently I can not find the way to retrieve domain
>>> number from pci_bus_assign_domain_nr (for those platforms which choose
>>> PCI_DOMAINS_GENERIC and want to use segment nr from MCFG table) which
>>> is the
>>> part of pci_create_root_bus.
>>
>> Not sure I fully understand your question, but
>> pci_bus_assign_domain_nr() will
>> put the assigned domain number in bus->domain_nr if you chose
>> PCI_DOMAINS_GENERIC.
>> Do you want to override that value with the segment nr from MCFG?
>>
> 
> Let me give ACPI ARM64 example:
> 
> 1. We parse MCFG table and get segment nr assigned to root bridge
> 2. Then PCI host bridge calls acpi_pci_root_create ->
> pci_create_root_bus -> pci_bus_assign_domain_nr
> 3. At this point we cannot get segment nr for ACPI
> 
> So I would like to assign MCFG segment nr to bus->domain_nr being in
> pci_bus_assign_domain_nr giving we have scenario above.
Please use sysdata for that, IA64 and x86 are making use of sysdata
to store such information:
struct pci_sysdata {
        int             domain;         /* PCI domain */
        int             node;           /* NUMA node */
#ifdef CONFIG_ACPI
        struct acpi_device *companion;  /* ACPI companion device */
#endif
#ifdef CONFIG_X86_64
        void            *iommu;         /* IOMMU private data */
#endif
};

Hanjun once tried to introduce struct pci_sysdata for ARM64, but
seems it has been rejected.

> 
> Thanks,
> Tomasz
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Oct 21, 2015 at 07:49:13PM +0800, Jiang Liu wrote:
> On 2015/10/21 19:27, Tomasz Nowicki wrote:
> > On 21.10.2015 13:02, Liviu Dudau wrote:
> >> On Wed, Oct 21, 2015 at 11:57:53AM +0200, 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 <lorenzo.pieralisi@arm.com>
> >>>> Tested-by: Tony Luck <tony.luck@intel.com>
> >>>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> >>>> Signed-off-by: Liu Jiang <jiang.liu@linux.intel.com>
> >>>> ---
> >>>>   drivers/acpi/pci_root.c  |  204
> >>>> ++++++++++++++++++++++++++++++++++++++++++++++
> >>>>   include/linux/pci-acpi.h |   24 ++++++
> >>>>   2 files changed, 228 insertions(+)
> >>>>
> >>>
> >>> [...]
> >>>
> >>>> +
> >>>> +struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> >>>> +                     struct acpi_pci_root_ops *ops,
> >>>> +                     struct acpi_pci_root_info *info,
> >>>> +                     void *sysdata)
> >>>> +{
> >>>> +    int ret, busnum = root->secondary.start;
> >>>> +    struct acpi_device *device = root->device;
> >>>> +    int node = acpi_get_node(device->handle);
> >>>> +    struct pci_bus *bus;
> >>>> +
> >>>> +    info->root = root;
> >>>> +    info->bridge = device;
> >>>> +    info->ops = ops;
> >>>> +    INIT_LIST_HEAD(&info->resources);
> >>>> +    snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
> >>>> +         root->segment, busnum);
> >>>> +
> >>>> +    if (ops->init_info && ops->init_info(info))
> >>>> +        goto out_release_info;
> >>>> +    if (ops->prepare_resources)
> >>>> +        ret = ops->prepare_resources(info);
> >>>> +    else
> >>>> +        ret = acpi_pci_probe_root_resources(info);
> >>>> +    if (ret < 0)
> >>>> +        goto out_release_info;
> >>>> +
> >>>> +    pci_acpi_root_add_resources(info);
> >>>> +    pci_add_resource(&info->resources, &root->secondary);
> >>>> +    bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
> >>>> +                  sysdata, &info->resources);
> >>>
> >>> Thank a lot for this cleanup!!
> >>>
> >>> I recall you already considered passing segment (domain nr) to
> >>> pci_create_root_bus, right? Can you please remind me why we gave up
> >>> on this?
> >>>
> >>> I am asking because currently I can not find the way to retrieve domain
> >>> number from pci_bus_assign_domain_nr (for those platforms which choose
> >>> PCI_DOMAINS_GENERIC and want to use segment nr from MCFG table) which
> >>> is the
> >>> part of pci_create_root_bus.
> >>
> >> Not sure I fully understand your question, but
> >> pci_bus_assign_domain_nr() will
> >> put the assigned domain number in bus->domain_nr if you chose
> >> PCI_DOMAINS_GENERIC.
> >> Do you want to override that value with the segment nr from MCFG?
> >>
> > 
> > Let me give ACPI ARM64 example:
> > 
> > 1. We parse MCFG table and get segment nr assigned to root bridge
> > 2. Then PCI host bridge calls acpi_pci_root_create ->
> > pci_create_root_bus -> pci_bus_assign_domain_nr
> > 3. At this point we cannot get segment nr for ACPI
> > 
> > So I would like to assign MCFG segment nr to bus->domain_nr being in
> > pci_bus_assign_domain_nr giving we have scenario above.
> Please use sysdata for that, IA64 and x86 are making use of sysdata
> to store such information:
> struct pci_sysdata {
>         int             domain;         /* PCI domain */
>         int             node;           /* NUMA node */
> #ifdef CONFIG_ACPI
>         struct acpi_device *companion;  /* ACPI companion device */
> #endif
> #ifdef CONFIG_X86_64
>         void            *iommu;         /* IOMMU private data */
> #endif
> };
> 
> Hanjun once tried to introduce struct pci_sysdata for ARM64, but
> seems it has been rejected.

For good reason! There is a lot of duplication between different arches notion
of the pci_sysdata and they can be moved into pci_bus or pci_host_bridge
structures. The goal is to get rid of multiple pci_sysdata structures, not
to add more.

Best regards,
Liviu

> 
> > 
> > Thanks,
> > Tomasz
> > -- 
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
>

On 21.10.2015 13:42, Lorenzo Pieralisi wrote:
> On Wed, Oct 21, 2015 at 01:27:33PM +0200, Tomasz Nowicki wrote:
>> On 21.10.2015 13:02, Liviu Dudau wrote:
>>> On Wed, Oct 21, 2015 at 11:57:53AM +0200, 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 <lorenzo.pieralisi@arm.com>
>>>>> Tested-by: Tony Luck <tony.luck@intel.com>
>>>>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
>>>>> Signed-off-by: Liu Jiang <jiang.liu@linux.intel.com>
>>>>> ---
>>>>>   drivers/acpi/pci_root.c  |  204 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>   include/linux/pci-acpi.h |   24 ++++++
>>>>>   2 files changed, 228 insertions(+)
>>>>>
>>>>
>>>> [...]
>>>>
>>>>> +
>>>>> +struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>>>>> +				     struct acpi_pci_root_ops *ops,
>>>>> +				     struct acpi_pci_root_info *info,
>>>>> +				     void *sysdata)
>>>>> +{
>>>>> +	int ret, busnum = root->secondary.start;
>>>>> +	struct acpi_device *device = root->device;
>>>>> +	int node = acpi_get_node(device->handle);
>>>>> +	struct pci_bus *bus;
>>>>> +
>>>>> +	info->root = root;
>>>>> +	info->bridge = device;
>>>>> +	info->ops = ops;
>>>>> +	INIT_LIST_HEAD(&info->resources);
>>>>> +	snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
>>>>> +		 root->segment, busnum);
>>>>> +
>>>>> +	if (ops->init_info && ops->init_info(info))
>>>>> +		goto out_release_info;
>>>>> +	if (ops->prepare_resources)
>>>>> +		ret = ops->prepare_resources(info);
>>>>> +	else
>>>>> +		ret = acpi_pci_probe_root_resources(info);
>>>>> +	if (ret < 0)
>>>>> +		goto out_release_info;
>>>>> +
>>>>> +	pci_acpi_root_add_resources(info);
>>>>> +	pci_add_resource(&info->resources, &root->secondary);
>>>>> +	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
>>>>> +				  sysdata, &info->resources);
>>>>
>>>> Thank a lot for this cleanup!!
>>>>
>>>> I recall you already considered passing segment (domain nr) to
>>>> pci_create_root_bus, right? Can you please remind me why we gave up on this?
>>>>
>>>> I am asking because currently I can not find the way to retrieve domain
>>>> number from pci_bus_assign_domain_nr (for those platforms which choose
>>>> PCI_DOMAINS_GENERIC and want to use segment nr from MCFG table) which is the
>>>> part of pci_create_root_bus.
>>>
>>> Not sure I fully understand your question, but pci_bus_assign_domain_nr() will
>>> put the assigned domain number in bus->domain_nr if you chose PCI_DOMAINS_GENERIC.
>>> Do you want to override that value with the segment nr from MCFG?
>>>
>>
>> Let me give ACPI ARM64 example:
>>
>> 1. We parse MCFG table and get segment nr assigned to root bridge
>> 2. Then PCI host bridge calls acpi_pci_root_create ->
>> pci_create_root_bus -> pci_bus_assign_domain_nr
>> 3. At this point we cannot get segment nr for ACPI
>>
>> So I would like to assign MCFG segment nr to bus->domain_nr being in
>> pci_bus_assign_domain_nr giving we have scenario above.
>
> I do not understand what you mean by "assign MCFG segment" here, please
> explain. The MCFG segment group number is used to look-up the configuration
> space for a given host bridge, not to assign a domain number to it.
>
> The domain_nr above is the value returned by the _SEG object for the host
> bridge device.

If I read the code correctly, MCFG region assigned to PCI host bridge X 
should have the same value for its segment field and corresponding _SEG 
object. Anyway, you are right, the MCFG segment group number should be 
used to look-up the configuration space and _SEG object to retrieve 
domain nr.

To evaluate _SEG object being in pci_bus_assign_domain_nr we still miss 
there the PCI host bridge device. So I should change my previous 
question, can we pass down the PCI host bridge device to be able to call 
_SEG object being in pci_bus_assign_domain_nr?

Thanks,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 <lorenzo.pieralisi@arm.com>
> Tested-by: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> Signed-off-by: Liu Jiang <jiang.liu@linux.intel.com>
> ---
>   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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 <lorenzo.pieralisi@arm.com>
>> Tested-by: Tony Luck <tony.luck@intel.com>
>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
>> Signed-off-by: Liu Jiang <jiang.liu@linux.intel.com>
>> ---
>>   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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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



--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 06.11.2015 12:46, Jiang Liu wrote:
> On 2015/11/6 18:37, Tomasz Nowicki wrote:
>> On 06.11.2015 09:52, Jiang Liu wrote:
>>> On 2015/11/6 2:19, Lorenzo Pieralisi wrote:
>>>> On Thu, Nov 05, 2015 at 03:21:34PM +0100, Tomasz Nowicki wrote:
>>>>> On 14.10.2015 08:29, Jiang Liu wrote:
>>>>
>>>> [...]
>>>>
>>>>>> +static void acpi_pci_root_validate_resources(struct device *dev,
>>>>>> +                         struct list_head *resources,
>>>>>> +                         unsigned long type)
>>>>>> +{
>>>>>> +    LIST_HEAD(list);
>>>>>> +    struct resource *res1, *res2, *root = NULL;
>>>>>> +    struct resource_entry *tmp, *entry, *entry2;
>>>>>> +
>>>>>> +    BUG_ON((type & (IORESOURCE_MEM | IORESOURCE_IO)) == 0);
>>>>>> +    root = (type & IORESOURCE_MEM) ? &iomem_resource :
>>>>>> &ioport_resource;
>>>>>> +
>>>>>> +    list_splice_init(resources, &list);
>>>>>> +    resource_list_for_each_entry_safe(entry, tmp, &list) {
>>>>>> +        bool free = false;
>>>>>> +        resource_size_t end;
>>>>>> +
>>>>>> +        res1 = entry->res;
>>>>>> +        if (!(res1->flags & type))
>>>>>> +            goto next;
>>>>>> +
>>>>>> +        /* Exclude non-addressable range or non-addressable
>>>>>> portion */
>>>>>> +        end = min(res1->end, root->end);
>>>>>> +        if (end <= res1->start) {
>>>>>> +            dev_info(dev, "host bridge window %pR (ignored, not
>>>>>> CPU addressable)\n",
>>>>>> +                 res1);
>>>>>> +            free = true;
>>>>>> +            goto next;
>>>>>> +        } else if (res1->end != end) {
>>>>>> +            dev_info(dev, "host bridge window %pR ([%#llx-%#llx]
>>>>>> ignored, not CPU addressable)\n",
>>>>>> +                 res1, (unsigned long long)end + 1,
>>>>>> +                 (unsigned long long)res1->end);
>>>>>> +            res1->end = end;
>>>>>> +        }
>>>>>> +
>>>>>> +        resource_list_for_each_entry(entry2, resources) {
>>>>>> +            res2 = entry2->res;
>>>>>> +            if (!(res2->flags & type))
>>>>>> +                continue;
>>>>>> +
>>>>>> +            /*
>>>>>> +             * I don't like throwing away windows because then
>>>>>> +             * our resources no longer match the ACPI _CRS, but
>>>>>> +             * the kernel resource tree doesn't allow overlaps.
>>>>>> +             */
>>>>>> +            if (resource_overlaps(res1, res2)) {
>>>>>> +                res2->start = min(res1->start, res2->start);
>>>>>> +                res2->end = max(res1->end, res2->end);
>>>>>> +                dev_info(dev, "host bridge window expanded to %pR;
>>>>>> %pR ignored\n",
>>>>>> +                     res2, res1);
>>>>>> +                free = true;
>>>>>> +                goto next;
>>>>>> +            }
>>>>>> +        }
>>>>>> +
>>>>>> +next:
>>>>>> +        resource_list_del(entry);
>>>>>> +        if (free)
>>>>>> +            resource_list_free_entry(entry);
>>>>>> +        else
>>>>>> +            resource_list_add_tail(entry, resources);
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>> +int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
>>>>>> +{
>>>>>> +    int ret;
>>>>>> +    struct list_head *list = &info->resources;
>>>>>> +    struct acpi_device *device = info->bridge;
>>>>>> +    struct resource_entry *entry, *tmp;
>>>>>> +    unsigned long flags;
>>>>>> +
>>>>>> +    flags = IORESOURCE_IO | IORESOURCE_MEM |
>>>>>> IORESOURCE_MEM_8AND16BIT;
>>>>>> +    ret = acpi_dev_get_resources(device, list,
>>>>>> +                     acpi_dev_filter_resource_type_cb,
>>>>>> +                     (void *)flags);
>>>>>> +    if (ret < 0)
>>>>>> +        dev_warn(&device->dev,
>>>>>> +             "failed to parse _CRS method, error code %d\n", ret);
>>>>>> +    else if (ret == 0)
>>>>>> +        dev_dbg(&device->dev,
>>>>>> +            "no IO and memory resources present in _CRS\n");
>>>>>> +    else {
>>>>>> +        resource_list_for_each_entry_safe(entry, tmp, list) {
>>>>>> +            if (entry->res->flags & IORESOURCE_DISABLED)
>>>>>> +                resource_list_destroy_entry(entry);
>>>>>> +            else
>>>>>> +                entry->res->name = info->name;
>>>>>> +        }
>>>>>> +        acpi_pci_root_validate_resources(&device->dev, list,
>>>>>> +                         IORESOURCE_MEM);
>>>>>> +        acpi_pci_root_validate_resources(&device->dev, list,
>>>>>> +                         IORESOURCE_IO);
>>>>>
>>>>> It is not clear to me why we need these two calls above ^^^. We are
>>>>> using pci_acpi_root_add_resources(info) later. Is it not enough?
>>>>>
>>>>> Also, I cannot use acpi_pci_probe_root_resources() in my ARM64 PCI
>>>>> driver. It is because acpi_dev_get_resources is adding
>>>>> translation_offset to IO ranges start address and then:
>>>>> acpi_pci_root_validate_resources(&device->dev, list,
>>>>>                   IORESOURCE_IO);
>>>>> rejects that IO regions as it is out of my 0x0-SZ_16M window.
>>>>>
>>>>> Does acpi_pci_probe_root_resources meant to be x86 specific and I
>>>>> should avoid using it?
>>>>
>>>> IIUC, you _have_ to have the proper translation_offset to map the bridge
>>>> window into the IO address space:
>>>>
>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/348708.html
>>>>
>>>>
>>>> Then, using the offset, you should do something ia64 does, namely,
>>>> retrieve the CPU address corresponding to IO space (see
>>>> arch/ia64/pci/pci.c
>>>> - add_io_space()) and map it in the physical address space by using
>>>> pci_remap_iospace(), it is similar to what we have to do with DT.
>>>>
>>>> It is extremely confusing and I am not sure I got it right myself,
>>>> I am still grokking ia64 code to understand what it really does.
>>>>
>>>> So basically, the IO bridge window coming from acpi_dev_get_resource()
>>>> should represent the IO space in 0 - 16M, IIUC.
>>>>
>>>> By using the offset (that was initialized using translation_offset) and
>>>> the resource->start, you can retrieve the cpu address that you need to
>>>> actually map the IO space, since that's what we do on ARM (ie the
>>>> IO resource is an offset into the virtual address space set aside
>>>> for IO).
>>>>
>>>> Confusing, to say the least. Jiang, did I get it right ?
>>> Hi Lorenzo and Tomasz,
>>>      With a cup of coffee, I got myself awake eventually:)
>>> Now we are going to talk about IO port on IA64, really a little
>>> complex:( Actually there are two types of translation.
>>> 1) A PCI domain has a 24-bit IO port address space, there may
>>> be multiple IO port address spaces in systems with multiple PCI
>>> domains. So the first type of translation is to translate domain
>>> specific IO port address into system global IO port address
>>> (iomem_resource) by
>>>     res->start = acpi_des->start + acpi_des->translation_offset
>>>
>>> 2) IA64 needs to map IO port address spaces into MMIO address
>>> space because it has no instructions to access IO ports directly.
>>> So IA64 has reserved a MMIO range to map IO port address spaces.
>>> This type of translation relies on architecture specific information
>>> instead of ACPI descriptors.
>>>
>>> On the other hand, ACPI specification has defined  "I/O to Memory
>>> Translation" flag and "Memory to I/O Translation" flag in
>>> ACPI Extended Address Space Descriptor,
>>
>> Based on 2) and above, does it mean IA64 should use "ACPI Extended
>> Address Space Descriptor" for its PCI bridge IO windows only?
>>
>>   but current implementation
>>> doesn't really support such a use case. So we need to find a way
>>> out here. Could you please help to provide more information about
>>> PCI host bridge resource descriptor implementation details on
>>> ARM64?
>>>
>>
>> Sure, ARM64 (0-16M IO space) QEMU example:
>> DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
>>           0x00000000,         // Granularity
>>           0x00000000,         // Range Minimum
>>           0x0000FFFF,         // Range Maximum
>>           0x3EFF0000,         // Translation Offset
>>           0x00010000,         // Length
>>           ,, , TypeStatic)
> The above DWordIO resource descriptor doesn't confirm to the ACPI spec.
> According to my understanding, ARM/ARM64 has no concept of IO port
> address space, so the PCI host bridge will map IO port on PCI side
> onto MMIO on host side. In other words, PCI host bridge on ARM64
> implement a IO Port->MMIO translation instead of a IO Port->IO Port
> translation. If that's true, it should use 'TypeTranslation' instead
> of 'TypeStatic'. And kernel ACPI resource parsing interface doesn't
> support 'TypeTranslation' yet, so we need to find a solution for it.

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

Thanks,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
> > 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html