[V1,11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

Commit Message

Tomasz Nowicki Oct. 27, 2015, 4:38 p.m. UTC

Because of two patch series:
1. Jiang Liu's common interface to support PCI host bridge init
2. Refactoring of MMCONFIG, part of this patch set
now we can think about PCI buses enumeration for ARM64 and ACPI tables.

This patch introduce ACPI based PCI hostbridge init calls which
use information from MCFG table (PCI config space regions) and
_CRS (IO/irq resources) to initialize PCI hostbridge.

Signed-off-by: Tomasz Nowicki <[e-mail]>
Signed-off-by: Hanjun Guo <[e-mail]>
Signed-off-by: Suravee Suthikulpanit <[e-mail]>
CC: Arnd Bergmann <[e-mail]>
CC: Catalin Marinas <[e-mail]>
CC: Liviu Dudau <[e-mail]>
CC: Lorenzo Pieralisi <[e-mail]>
CC: Will Deacon <[e-mail]>
---
 arch/arm64/Kconfig      |   6 ++
 arch/arm64/kernel/pci.c | 208 +++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 202 insertions(+), 12 deletions(-)

Comments

[e-mail] Oct. 28, 2015, 11:49 a.m. UTC | #1

On Tue, Oct 27, 2015 at 05:38:42PM +0100, Tomasz Nowicki wrote:
> Because of two patch series:
> 1. Jiang Liu's common interface to support PCI host bridge init
> 2. Refactoring of MMCONFIG, part of this patch set
> now we can think about PCI buses enumeration for ARM64 and ACPI tables.
> 
> This patch introduce ACPI based PCI hostbridge init calls which
> use information from MCFG table (PCI config space regions) and
> _CRS (IO/irq resources) to initialize PCI hostbridge.
> 
> Signed-off-by: Tomasz Nowicki <[e-mail]>
> Signed-off-by: Hanjun Guo <[e-mail]>
> Signed-off-by: Suravee Suthikulpanit <[e-mail]>
> CC: Arnd Bergmann <[e-mail]>
> CC: Catalin Marinas <[e-mail]>
> CC: Liviu Dudau <[e-mail]>
> CC: Lorenzo Pieralisi <[e-mail]>
> CC: Will Deacon <[e-mail]>
> ---
>  arch/arm64/Kconfig      |   6 ++
>  arch/arm64/kernel/pci.c | 208 +++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 202 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 07d1811..bbcc6b1 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -89,6 +89,7 @@ config ARM64
>  	select SPARSE_IRQ
>  	select SYSCTL_EXCEPTION_TRACE
>  	select HAVE_CONTEXT_TRACKING
> +	select HAVE_PCI_ECAM
>  	help
>  	  ARM 64-bit (AArch64) Linux support.
>  
> @@ -202,6 +203,11 @@ source "drivers/pci/Kconfig"
>  source "drivers/pci/pcie/Kconfig"
>  source "drivers/pci/hotplug/Kconfig"
>  
> +config PCI_MMCONFIG
> +	def_bool y
> +	select PCI_ECAM
> +	depends on ACPI
> +
>  endmenu
>  
>  menu "Kernel Features"
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index b3d098b..66cc1ae 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -11,12 +11,15 @@
>   */
>  
>  #include <linux/acpi.h>
> +#include <linux/ecam.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/mm.h>
> +#include <linux/of_address.h>
>  #include <linux/of_pci.h>
>  #include <linux/of_platform.h>
> +#include <linux/pci-acpi.h>
>  #include <linux/slab.h>
>  
>  #include <asm/pci-bridge.h>
> @@ -52,35 +55,216 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
>  }
>  
>  /*
> - * Try to assign the IRQ number from DT when adding a new device
> + * Try to assign the IRQ number from DT/ACPI when adding a new device
>   */
>  int pcibios_add_device(struct pci_dev *dev)
>  {
> -	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> +	if (acpi_disabled)
> +		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> +#ifdef CONFIG_ACPI
> +	else
> +		acpi_pci_irq_enable(dev);
> +#endif
>  
>  	return 0;
>  }
>  
> +#ifdef CONFIG_ACPI
> +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> +{
> +	struct acpi_pci_root *root = bridge->bus->sysdata;
> +
> +	ACPI_COMPANION_SET(&bridge->dev, root->device);
> +	return 0;
> +}
> +
> +void pcibios_add_bus(struct pci_bus *bus)
> +{
> +	acpi_pci_add_bus(bus);
> +}
> +
> +void pcibios_remove_bus(struct pci_bus *bus)
> +{
> +	acpi_pci_remove_bus(bus);
> +}
> +
> +static int __init pcibios_assign_resources(void)
> +{
> +	if (acpi_disabled)
> +		return 0;
> +
> +	pci_assign_unassigned_resources();
> +	return 0;

You can change this function into:
{
	if (!acpi_disabled)
		pci_assign_unassigned_resources();

	return 0;
}

as the equivalent but shorter form.

> +}
>  /*
> - * raw_pci_read/write - Platform-specific PCI config space access.
> + * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
> + * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
>   */
> -int raw_pci_read(unsigned int domain, unsigned int bus,
> -		  unsigned int devfn, int reg, int len, u32 *val)

What happened with raw_pci_{read,write} ? Why do you remove them?


> +rootfs_initcall(pcibios_assign_resources);

Would you be so kind and explain to me why you need this initcall?
Can you not set the PCI_REASSIGN_ALL_RSRC flag before calling
pci_scan_root_bus() ?

I haven't focused on ACPI before so I'm a bit hazy on the init order when
that is enabled. That being said, I don't like adding in the architecture
code initcall hooks just to fix up some dependency orders that could / should
be fixed some other way.

> +
> +static void __iomem *
> +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
>  {
> -	return -ENXIO;
> +	struct pci_mmcfg_region *cfg;
> +
> +	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
> +	if (cfg && cfg->virt)
> +		return cfg->virt +
> +			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
> +			offset;
> +	return NULL;
>  }
>  
> -int raw_pci_write(unsigned int domain, unsigned int bus,
> -		unsigned int devfn, int reg, int len, u32 val)
> +struct pci_ops pci_root_ops = {
> +	.map_bus = pci_mcfg_dev_base,
> +	.read = pci_generic_config_read,
> +	.write = pci_generic_config_write,
> +};
> +
> +#ifdef CONFIG_PCI_MMCONFIG
> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
>  {
> -	return -ENXIO;
> +	struct pci_mmcfg_region *cfg;
> +	struct acpi_pci_root *root;
> +	int seg, start, end, err;
> +
> +	root = ci->root;
> +	seg = root->segment;
> +	start = root->secondary.start;
> +	end = root->secondary.end;
> +
> +	cfg = pci_mmconfig_lookup(seg, start);
> +	if (cfg)
> +		return 0;
> +
> +	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
> +	if (!cfg)
> +		return -ENOMEM;
> +
> +	err = pci_mmconfig_inject(cfg);
> +	return err;
>  }
>  
> -#ifdef CONFIG_ACPI
> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
> +{
> +	struct acpi_pci_root *root = ci->root;
> +	struct pci_mmcfg_region *cfg;
> +
> +	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
> +	if (cfg)
> +		return;
> +
> +	if (cfg->hot_added)
> +		pci_mmconfig_delete(root->segment, root->secondary.start,
> +				    root->secondary.end);
> +}
> +#else
> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> +{
> +	return 0;
> +}
> +
> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
> +#endif
> +
> +static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
> +{
> +	return pci_add_mmconfig_region(ci);
> +}
> +
> +static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
> +{
> +	pci_remove_mmconfig_region(ci);
> +	kfree(ci);
> +}
> +
> +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> +{
> +	struct resource_entry *entry, *tmp;
> +	int ret;
> +
> +	ret = acpi_pci_probe_root_resources(ci);
> +	if (ret < 0)
> +		return ret;
> +
> +	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
> +		struct resource *res = entry->res;
> +
> +		/*
> +		 * Special handling for ARM IO range

There is nothing ARM specific here. It should apply to any memory mapped IO range.

> +		 * 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;
> +
> +			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 - 1;
> +
> +			if (pci_remap_iospace(res, res->start) < 0)
> +				resource_list_destroy_entry(entry);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static struct acpi_pci_root_ops acpi_pci_root_ops = {
> +	.pci_ops = &pci_root_ops,
> +	.init_info = pci_acpi_root_init_info,
> +	.release_info = pci_acpi_root_release_info,
> +	.prepare_resources = pci_acpi_root_prepare_resources,
> +};
> +
>  /* Root bridge scanning */
>  struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  {
> -	/* TODO: Should be revisited when implementing PCI on ACPI */
> -	return NULL;
> +	int node = acpi_get_node(root->device->handle);
> +	int domain = root->segment;
> +	int busnum = root->secondary.start;
> +	struct acpi_pci_root_info *info;
> +	struct pci_bus *bus;
> +
> +	if (domain && !pci_domains_supported) {
> +		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
> +			domain, busnum);
> +		return NULL;
> +	}
> +
> +	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
> +	if (!info) {
> +		dev_err(&root->device->dev,
> +			"pci_bus %04x:%02x: ignored (out of memory)\n",
> +			domain, busnum);
> +		return NULL;
> +	}
> +
> +	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
> +
> +	/* After the PCI-E bus has been walked and all devices discovered,
> +	 * configure any settings of the fabric that might be necessary.
> +	 */
> +	if (bus) {
> +		struct pci_bus *child;
> +
> +		list_for_each_entry(child, &bus->children, node)
> +			pcie_bus_configure_settings(child);
> +	}
> +
> +	return bus;
>  }
>  #endif
> -- 
> 1.9.1
>

Tomasz Nowicki Oct. 28, 2015, 1:42 p.m. UTC | #2

On 28.10.2015 12:49, [e-mail] wrote:
> On Tue, Oct 27, 2015 at 05:38:42PM +0100, Tomasz Nowicki wrote:
>> Because of two patch series:
>> 1. Jiang Liu's common interface to support PCI host bridge init
>> 2. Refactoring of MMCONFIG, part of this patch set
>> now we can think about PCI buses enumeration for ARM64 and ACPI tables.
>>
>> This patch introduce ACPI based PCI hostbridge init calls which
>> use information from MCFG table (PCI config space regions) and
>> _CRS (IO/irq resources) to initialize PCI hostbridge.
>>
>> Signed-off-by: Tomasz Nowicki <[e-mail]>
>> Signed-off-by: Hanjun Guo <[e-mail]>
>> Signed-off-by: Suravee Suthikulpanit <[e-mail]>
>> CC: Arnd Bergmann <[e-mail]>
>> CC: Catalin Marinas <[e-mail]>
>> CC: Liviu Dudau <[e-mail]>
>> CC: Lorenzo Pieralisi <[e-mail]>
>> CC: Will Deacon <[e-mail]>
>> ---
>>   arch/arm64/Kconfig      |   6 ++
>>   arch/arm64/kernel/pci.c | 208 +++++++++++++++++++++++++++++++++++++++++++++---
>>   2 files changed, 202 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 07d1811..bbcc6b1 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -89,6 +89,7 @@ config ARM64
>>   	select SPARSE_IRQ
>>   	select SYSCTL_EXCEPTION_TRACE
>>   	select HAVE_CONTEXT_TRACKING
>> +	select HAVE_PCI_ECAM
>>   	help
>>   	  ARM 64-bit (AArch64) Linux support.
>>
>> @@ -202,6 +203,11 @@ source "drivers/pci/Kconfig"
>>   source "drivers/pci/pcie/Kconfig"
>>   source "drivers/pci/hotplug/Kconfig"
>>
>> +config PCI_MMCONFIG
>> +	def_bool y
>> +	select PCI_ECAM
>> +	depends on ACPI
>> +
>>   endmenu
>>
>>   menu "Kernel Features"
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index b3d098b..66cc1ae 100644
>> --- a/arch/arm64/kernel/pci.c
>> +++ b/arch/arm64/kernel/pci.c
>> @@ -11,12 +11,15 @@
>>    */
>>
>>   #include <linux/acpi.h>
>> +#include <linux/ecam.h>
>>   #include <linux/init.h>
>>   #include <linux/io.h>
>>   #include <linux/kernel.h>
>>   #include <linux/mm.h>
>> +#include <linux/of_address.h>
>>   #include <linux/of_pci.h>
>>   #include <linux/of_platform.h>
>> +#include <linux/pci-acpi.h>
>>   #include <linux/slab.h>
>>
>>   #include <asm/pci-bridge.h>
>> @@ -52,35 +55,216 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
>>   }
>>
>>   /*
>> - * Try to assign the IRQ number from DT when adding a new device
>> + * Try to assign the IRQ number from DT/ACPI when adding a new device
>>    */
>>   int pcibios_add_device(struct pci_dev *dev)
>>   {
>> -	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
>> +	if (acpi_disabled)
>> +		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
>> +#ifdef CONFIG_ACPI
>> +	else
>> +		acpi_pci_irq_enable(dev);
>> +#endif
>>
>>   	return 0;
>>   }
>>
>> +#ifdef CONFIG_ACPI
>> +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
>> +{
>> +	struct acpi_pci_root *root = bridge->bus->sysdata;
>> +
>> +	ACPI_COMPANION_SET(&bridge->dev, root->device);
>> +	return 0;
>> +}
>> +
>> +void pcibios_add_bus(struct pci_bus *bus)
>> +{
>> +	acpi_pci_add_bus(bus);
>> +}
>> +
>> +void pcibios_remove_bus(struct pci_bus *bus)
>> +{
>> +	acpi_pci_remove_bus(bus);
>> +}
>> +
>> +static int __init pcibios_assign_resources(void)
>> +{
>> +	if (acpi_disabled)
>> +		return 0;
>> +
>> +	pci_assign_unassigned_resources();
>> +	return 0;
>
> You can change this function into:
> {
> 	if (!acpi_disabled)
> 		pci_assign_unassigned_resources();
>
> 	return 0;
> }
>
> as the equivalent but shorter form.

Sure, will do.

>
>> +}
>>   /*
>> - * raw_pci_read/write - Platform-specific PCI config space access.
>> + * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
>> + * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
>>    */
>> -int raw_pci_read(unsigned int domain, unsigned int bus,
>> -		  unsigned int devfn, int reg, int len, u32 *val)
>
> What happened with raw_pci_{read,write} ? Why do you remove them?

We do not need raw_pci_{read,write} any more, we will use empty 
raw_pci_{read,write} from mcfg.c introduced in patch 6/11.

I think this is another candidate to split up, first I will remove these 
raw_pci_{read,write} accessors and then introduce ACPI PCI hostbridge 
init, it will be easier to review, what do you think?

>
>
>> +rootfs_initcall(pcibios_assign_resources);
>
> Would you be so kind and explain to me why you need this initcall?
> Can you not set the PCI_REASSIGN_ALL_RSRC flag before calling
> pci_scan_root_bus() ?
>
> I haven't focused on ACPI before so I'm a bit hazy on the init order when
> that is enabled. That being said, I don't like adding in the architecture
> code initcall hooks just to fix up some dependency orders that could / should
> be fixed some other way.

My idea was to defer resource reassigning to give a chance for running 
DECLARE_PCI_FIXUP_FINAL. I used DECLARE_PCI_FIXUP_FINAL to set 
IORESOURCE_PCI_FIXED for some PCI devices. Now I am not sure we need to 
defer it, my DECLARE_PCI_FIXUP_FINALs might be done in firmware, let me 
get back to you on this.

>
>> +
>> +static void __iomem *
>> +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
>>   {
>> -	return -ENXIO;
>> +	struct pci_mmcfg_region *cfg;
>> +
>> +	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
>> +	if (cfg && cfg->virt)
>> +		return cfg->virt +
>> +			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
>> +			offset;
>> +	return NULL;
>>   }
>>
>> -int raw_pci_write(unsigned int domain, unsigned int bus,
>> -		unsigned int devfn, int reg, int len, u32 val)
>> +struct pci_ops pci_root_ops = {
>> +	.map_bus = pci_mcfg_dev_base,
>> +	.read = pci_generic_config_read,
>> +	.write = pci_generic_config_write,
>> +};
>> +
>> +#ifdef CONFIG_PCI_MMCONFIG
>> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
>>   {
>> -	return -ENXIO;
>> +	struct pci_mmcfg_region *cfg;
>> +	struct acpi_pci_root *root;
>> +	int seg, start, end, err;
>> +
>> +	root = ci->root;
>> +	seg = root->segment;
>> +	start = root->secondary.start;
>> +	end = root->secondary.end;
>> +
>> +	cfg = pci_mmconfig_lookup(seg, start);
>> +	if (cfg)
>> +		return 0;
>> +
>> +	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
>> +	if (!cfg)
>> +		return -ENOMEM;
>> +
>> +	err = pci_mmconfig_inject(cfg);
>> +	return err;
>>   }
>>
>> -#ifdef CONFIG_ACPI
>> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
>> +{
>> +	struct acpi_pci_root *root = ci->root;
>> +	struct pci_mmcfg_region *cfg;
>> +
>> +	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
>> +	if (cfg)
>> +		return;
>> +
>> +	if (cfg->hot_added)
>> +		pci_mmconfig_delete(root->segment, root->secondary.start,
>> +				    root->secondary.end);
>> +}
>> +#else
>> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
>> +{
>> +	return 0;
>> +}
>> +
>> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
>> +#endif
>> +
>> +static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
>> +{
>> +	return pci_add_mmconfig_region(ci);
>> +}
>> +
>> +static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
>> +{
>> +	pci_remove_mmconfig_region(ci);
>> +	kfree(ci);
>> +}
>> +
>> +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
>> +{
>> +	struct resource_entry *entry, *tmp;
>> +	int ret;
>> +
>> +	ret = acpi_pci_probe_root_resources(ci);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
>> +		struct resource *res = entry->res;
>> +
>> +		/*
>> +		 * Special handling for ARM IO range
>
> There is nothing ARM specific here. It should apply to any memory mapped IO range.

OK, I will remove that comment.

>
>> +		 * 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;
>> +
>> +			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 - 1;
>> +
>> +			if (pci_remap_iospace(res, res->start) < 0)
>> +				resource_list_destroy_entry(entry);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct acpi_pci_root_ops acpi_pci_root_ops = {
>> +	.pci_ops = &pci_root_ops,
>> +	.init_info = pci_acpi_root_init_info,
>> +	.release_info = pci_acpi_root_release_info,
>> +	.prepare_resources = pci_acpi_root_prepare_resources,
>> +};
>> +
>>   /* Root bridge scanning */
>>   struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>>   {
>> -	/* TODO: Should be revisited when implementing PCI on ACPI */
>> -	return NULL;
>> +	int node = acpi_get_node(root->device->handle);
>> +	int domain = root->segment;
>> +	int busnum = root->secondary.start;
>> +	struct acpi_pci_root_info *info;
>> +	struct pci_bus *bus;
>> +
>> +	if (domain && !pci_domains_supported) {
>> +		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
>> +			domain, busnum);
>> +		return NULL;
>> +	}
>> +
>> +	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
>> +	if (!info) {
>> +		dev_err(&root->device->dev,
>> +			"pci_bus %04x:%02x: ignored (out of memory)\n",
>> +			domain, busnum);
>> +		return NULL;
>> +	}
>> +
>> +	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
>> +
>> +	/* After the PCI-E bus has been walked and all devices discovered,
>> +	 * configure any settings of the fabric that might be necessary.
>> +	 */
>> +	if (bus) {
>> +		struct pci_bus *child;
>> +
>> +		list_for_each_entry(child, &bus->children, node)
>> +			pcie_bus_configure_settings(child);
>> +	}
>> +
>> +	return bus;
>>   }
>>   #endif
>> --
>> 1.9.1
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[e-mail] Oct. 28, 2015, 1:51 p.m. UTC | #3

On Wed, Oct 28, 2015 at 02:42:30PM +0100, Tomasz Nowicki wrote:
> On 28.10.2015 12:49, [e-mail] wrote:
> >On Tue, Oct 27, 2015 at 05:38:42PM +0100, Tomasz Nowicki wrote:
> >>Because of two patch series:
> >>1. Jiang Liu's common interface to support PCI host bridge init
> >>2. Refactoring of MMCONFIG, part of this patch set
> >>now we can think about PCI buses enumeration for ARM64 and ACPI tables.
> >>
> >>This patch introduce ACPI based PCI hostbridge init calls which
> >>use information from MCFG table (PCI config space regions) and
> >>_CRS (IO/irq resources) to initialize PCI hostbridge.
> >>
> >>Signed-off-by: Tomasz Nowicki <[e-mail]>
> >>Signed-off-by: Hanjun Guo <[e-mail]>
> >>Signed-off-by: Suravee Suthikulpanit <[e-mail]>
> >>CC: Arnd Bergmann <[e-mail]>
> >>CC: Catalin Marinas <[e-mail]>
> >>CC: Liviu Dudau <[e-mail]>
> >>CC: Lorenzo Pieralisi <[e-mail]>
> >>CC: Will Deacon <[e-mail]>
> >>---
> >>  arch/arm64/Kconfig      |   6 ++
> >>  arch/arm64/kernel/pci.c | 208 +++++++++++++++++++++++++++++++++++++++++++++---
> >>  2 files changed, 202 insertions(+), 12 deletions(-)
> >>
> >>diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> >>index 07d1811..bbcc6b1 100644
> >>--- a/arch/arm64/Kconfig
> >>+++ b/arch/arm64/Kconfig
> >>@@ -89,6 +89,7 @@ config ARM64
> >>  	select SPARSE_IRQ
> >>  	select SYSCTL_EXCEPTION_TRACE
> >>  	select HAVE_CONTEXT_TRACKING
> >>+	select HAVE_PCI_ECAM
> >>  	help
> >>  	  ARM 64-bit (AArch64) Linux support.
> >>
> >>@@ -202,6 +203,11 @@ source "drivers/pci/Kconfig"
> >>  source "drivers/pci/pcie/Kconfig"
> >>  source "drivers/pci/hotplug/Kconfig"
> >>
> >>+config PCI_MMCONFIG
> >>+	def_bool y
> >>+	select PCI_ECAM
> >>+	depends on ACPI
> >>+
> >>  endmenu
> >>
> >>  menu "Kernel Features"
> >>diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> >>index b3d098b..66cc1ae 100644
> >>--- a/arch/arm64/kernel/pci.c
> >>+++ b/arch/arm64/kernel/pci.c
> >>@@ -11,12 +11,15 @@
> >>   */
> >>
> >>  #include <linux/acpi.h>
> >>+#include <linux/ecam.h>
> >>  #include <linux/init.h>
> >>  #include <linux/io.h>
> >>  #include <linux/kernel.h>
> >>  #include <linux/mm.h>
> >>+#include <linux/of_address.h>
> >>  #include <linux/of_pci.h>
> >>  #include <linux/of_platform.h>
> >>+#include <linux/pci-acpi.h>
> >>  #include <linux/slab.h>
> >>
> >>  #include <asm/pci-bridge.h>
> >>@@ -52,35 +55,216 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
> >>  }
> >>
> >>  /*
> >>- * Try to assign the IRQ number from DT when adding a new device
> >>+ * Try to assign the IRQ number from DT/ACPI when adding a new device
> >>   */
> >>  int pcibios_add_device(struct pci_dev *dev)
> >>  {
> >>-	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> >>+	if (acpi_disabled)
> >>+		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> >>+#ifdef CONFIG_ACPI
> >>+	else
> >>+		acpi_pci_irq_enable(dev);
> >>+#endif
> >>
> >>  	return 0;
> >>  }
> >>
> >>+#ifdef CONFIG_ACPI
> >>+int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> >>+{
> >>+	struct acpi_pci_root *root = bridge->bus->sysdata;
> >>+
> >>+	ACPI_COMPANION_SET(&bridge->dev, root->device);
> >>+	return 0;
> >>+}
> >>+
> >>+void pcibios_add_bus(struct pci_bus *bus)
> >>+{
> >>+	acpi_pci_add_bus(bus);
> >>+}
> >>+
> >>+void pcibios_remove_bus(struct pci_bus *bus)
> >>+{
> >>+	acpi_pci_remove_bus(bus);
> >>+}
> >>+
> >>+static int __init pcibios_assign_resources(void)
> >>+{
> >>+	if (acpi_disabled)
> >>+		return 0;
> >>+
> >>+	pci_assign_unassigned_resources();
> >>+	return 0;
> >
> >You can change this function into:
> >{
> >	if (!acpi_disabled)
> >		pci_assign_unassigned_resources();
> >
> >	return 0;
> >}
> >
> >as the equivalent but shorter form.
> 
> Sure, will do.
> 
> >
> >>+}
> >>  /*
> >>- * raw_pci_read/write - Platform-specific PCI config space access.
> >>+ * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
> >>+ * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
> >>   */
> >>-int raw_pci_read(unsigned int domain, unsigned int bus,
> >>-		  unsigned int devfn, int reg, int len, u32 *val)
> >
> >What happened with raw_pci_{read,write} ? Why do you remove them?
> 
> We do not need raw_pci_{read,write} any more, we will use empty
> raw_pci_{read,write} from mcfg.c introduced in patch 6/11.
> 
> I think this is another candidate to split up, first I will remove these
> raw_pci_{read,write} accessors and then introduce ACPI PCI hostbridge init,
> it will be easier to review, what do you think?

Yes, I like that. If the calls were redundant after 6/11 then they should
not have been kept until this patch to be removed.

> 
> >
> >
> >>+rootfs_initcall(pcibios_assign_resources);
> >
> >Would you be so kind and explain to me why you need this initcall?
> >Can you not set the PCI_REASSIGN_ALL_RSRC flag before calling
> >pci_scan_root_bus() ?
> >
> >I haven't focused on ACPI before so I'm a bit hazy on the init order when
> >that is enabled. That being said, I don't like adding in the architecture
> >code initcall hooks just to fix up some dependency orders that could / should
> >be fixed some other way.
> 
> My idea was to defer resource reassigning to give a chance for running
> DECLARE_PCI_FIXUP_FINAL. I used DECLARE_PCI_FIXUP_FINAL to set
> IORESOURCE_PCI_FIXED for some PCI devices. Now I am not sure we need to
> defer it, my DECLARE_PCI_FIXUP_FINALs might be done in firmware, let me get
> back to you on this.

If fixups can be done in firmware then that is the preferred direction nowadays.

> 
> >
> >>+
> >>+static void __iomem *
> >>+pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
> >>  {
> >>-	return -ENXIO;
> >>+	struct pci_mmcfg_region *cfg;
> >>+
> >>+	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
> >>+	if (cfg && cfg->virt)
> >>+		return cfg->virt +
> >>+			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
> >>+			offset;
> >>+	return NULL;
> >>  }
> >>
> >>-int raw_pci_write(unsigned int domain, unsigned int bus,
> >>-		unsigned int devfn, int reg, int len, u32 val)
> >>+struct pci_ops pci_root_ops = {
> >>+	.map_bus = pci_mcfg_dev_base,
> >>+	.read = pci_generic_config_read,
> >>+	.write = pci_generic_config_write,
> >>+};
> >>+
> >>+#ifdef CONFIG_PCI_MMCONFIG
> >>+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> >>  {
> >>-	return -ENXIO;
> >>+	struct pci_mmcfg_region *cfg;
> >>+	struct acpi_pci_root *root;
> >>+	int seg, start, end, err;
> >>+
> >>+	root = ci->root;
> >>+	seg = root->segment;
> >>+	start = root->secondary.start;
> >>+	end = root->secondary.end;
> >>+
> >>+	cfg = pci_mmconfig_lookup(seg, start);
> >>+	if (cfg)
> >>+		return 0;
> >>+
> >>+	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
> >>+	if (!cfg)
> >>+		return -ENOMEM;
> >>+
> >>+	err = pci_mmconfig_inject(cfg);
> >>+	return err;
> >>  }
> >>
> >>-#ifdef CONFIG_ACPI
> >>+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
> >>+{
> >>+	struct acpi_pci_root *root = ci->root;
> >>+	struct pci_mmcfg_region *cfg;
> >>+
> >>+	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
> >>+	if (cfg)
> >>+		return;
> >>+
> >>+	if (cfg->hot_added)
> >>+		pci_mmconfig_delete(root->segment, root->secondary.start,
> >>+				    root->secondary.end);
> >>+}
> >>+#else
> >>+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> >>+{
> >>+	return 0;
> >>+}
> >>+
> >>+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
> >>+#endif
> >>+
> >>+static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
> >>+{
> >>+	return pci_add_mmconfig_region(ci);
> >>+}
> >>+
> >>+static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
> >>+{
> >>+	pci_remove_mmconfig_region(ci);
> >>+	kfree(ci);
> >>+}
> >>+
> >>+static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> >>+{
> >>+	struct resource_entry *entry, *tmp;
> >>+	int ret;
> >>+
> >>+	ret = acpi_pci_probe_root_resources(ci);
> >>+	if (ret < 0)
> >>+		return ret;
> >>+
> >>+	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
> >>+		struct resource *res = entry->res;
> >>+
> >>+		/*
> >>+		 * Special handling for ARM IO range
> >
> >There is nothing ARM specific here. It should apply to any memory mapped IO range.
> 
> OK, I will remove that comment.

Thanks!

Liviu

> 
> >
> >>+		 * 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;
> >>+
> >>+			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 - 1;
> >>+
> >>+			if (pci_remap_iospace(res, res->start) < 0)
> >>+				resource_list_destroy_entry(entry);
> >>+		}
> >>+	}
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>+static struct acpi_pci_root_ops acpi_pci_root_ops = {
> >>+	.pci_ops = &pci_root_ops,
> >>+	.init_info = pci_acpi_root_init_info,
> >>+	.release_info = pci_acpi_root_release_info,
> >>+	.prepare_resources = pci_acpi_root_prepare_resources,
> >>+};
> >>+
> >>  /* Root bridge scanning */
> >>  struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> >>  {
> >>-	/* TODO: Should be revisited when implementing PCI on ACPI */
> >>-	return NULL;
> >>+	int node = acpi_get_node(root->device->handle);
> >>+	int domain = root->segment;
> >>+	int busnum = root->secondary.start;
> >>+	struct acpi_pci_root_info *info;
> >>+	struct pci_bus *bus;
> >>+
> >>+	if (domain && !pci_domains_supported) {
> >>+		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
> >>+			domain, busnum);
> >>+		return NULL;
> >>+	}
> >>+
> >>+	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
> >>+	if (!info) {
> >>+		dev_err(&root->device->dev,
> >>+			"pci_bus %04x:%02x: ignored (out of memory)\n",
> >>+			domain, busnum);
> >>+		return NULL;
> >>+	}
> >>+
> >>+	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
> >>+
> >>+	/* After the PCI-E bus has been walked and all devices discovered,
> >>+	 * configure any settings of the fabric that might be necessary.
> >>+	 */
> >>+	if (bus) {
> >>+		struct pci_bus *child;
> >>+
> >>+		list_for_each_entry(child, &bus->children, node)
> >>+			pcie_bus_configure_settings(child);
> >>+	}
> >>+
> >>+	return bus;
> >>  }
> >>  #endif
> >>--
> >>1.9.1
> >>
> >
>

Sinan Kaya Oct. 28, 2015, 6:46 p.m. UTC | #4

On 10/27/2015 12:38 PM, Tomasz Nowicki wrote:
> Because of two patch series:
> 1. Jiang Liu's common interface to support PCI host bridge init
> 2. Refactoring of MMCONFIG, part of this patch set
> now we can think about PCI buses enumeration for ARM64 and ACPI tables.
>
> This patch introduce ACPI based PCI hostbridge init calls which
> use information from MCFG table (PCI config space regions) and
> _CRS (IO/irq resources) to initialize PCI hostbridge.
>
> Signed-off-by: Tomasz Nowicki <[e-mail]>
> Signed-off-by: Hanjun Guo <[e-mail]>
> Signed-off-by: Suravee Suthikulpanit <[e-mail]>
> CC: Arnd Bergmann <[e-mail]>
> CC: Catalin Marinas <[e-mail]>
> CC: Liviu Dudau <[e-mail]>
> CC: Lorenzo Pieralisi <[e-mail]>
> CC: Will Deacon <[e-mail]>
> ---
>   arch/arm64/Kconfig      |   6 ++
>   arch/arm64/kernel/pci.c | 208 +++++++++++++++++++++++++++++++++++++++++++++---
>   2 files changed, 202 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 07d1811..bbcc6b1 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -89,6 +89,7 @@ config ARM64
>   	select SPARSE_IRQ
>   	select SYSCTL_EXCEPTION_TRACE
>   	select HAVE_CONTEXT_TRACKING
> +	select HAVE_PCI_ECAM
>   	help
>   	  ARM 64-bit (AArch64) Linux support.
>
> @@ -202,6 +203,11 @@ source "drivers/pci/Kconfig"
>   source "drivers/pci/pcie/Kconfig"
>   source "drivers/pci/hotplug/Kconfig"
>
> +config PCI_MMCONFIG
> +	def_bool y
> +	select PCI_ECAM
> +	depends on ACPI
> +
>   endmenu
>
>   menu "Kernel Features"
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index b3d098b..66cc1ae 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -11,12 +11,15 @@
>    */
>
>   #include <linux/acpi.h>
> +#include <linux/ecam.h>
>   #include <linux/init.h>
>   #include <linux/io.h>
>   #include <linux/kernel.h>
>   #include <linux/mm.h>
> +#include <linux/of_address.h>
>   #include <linux/of_pci.h>
>   #include <linux/of_platform.h>
> +#include <linux/pci-acpi.h>
>   #include <linux/slab.h>
>
>   #include <asm/pci-bridge.h>
> @@ -52,35 +55,216 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
>   }
>
>   /*
> - * Try to assign the IRQ number from DT when adding a new device
> + * Try to assign the IRQ number from DT/ACPI when adding a new device
>    */
>   int pcibios_add_device(struct pci_dev *dev)
>   {
> -	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> +	if (acpi_disabled)
> +		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> +#ifdef CONFIG_ACPI
> +	else
> +		acpi_pci_irq_enable(dev);
> +#endif
>
>   	return 0;
>   }
>
> +#ifdef CONFIG_ACPI
> +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> +{
> +	struct acpi_pci_root *root = bridge->bus->sysdata;
> +
> +	ACPI_COMPANION_SET(&bridge->dev, root->device);
> +	return 0;
> +}
> +
> +void pcibios_add_bus(struct pci_bus *bus)
> +{
> +	acpi_pci_add_bus(bus);
> +}
> +
> +void pcibios_remove_bus(struct pci_bus *bus)
> +{
> +	acpi_pci_remove_bus(bus);
> +}
> +
> +static int __init pcibios_assign_resources(void)
> +{
> +	if (acpi_disabled)
> +		return 0;
> +
> +	pci_assign_unassigned_resources();
> +	return 0;
> +}
>   /*
> - * raw_pci_read/write - Platform-specific PCI config space access.
> + * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
> + * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
>    */
> -int raw_pci_read(unsigned int domain, unsigned int bus,
> -		  unsigned int devfn, int reg, int len, u32 *val)
> +rootfs_initcall(pcibios_assign_resources);
> +
> +static void __iomem *
> +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
>   {
> -	return -ENXIO;
> +	struct pci_mmcfg_region *cfg;
> +
> +	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
> +	if (cfg && cfg->virt)
> +		return cfg->virt +
> +			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
> +			offset;
> +	return NULL;
>   }
>
> -int raw_pci_write(unsigned int domain, unsigned int bus,
> -		unsigned int devfn, int reg, int len, u32 val)
> +struct pci_ops pci_root_ops = {
> +	.map_bus = pci_mcfg_dev_base,
> +	.read = pci_generic_config_read,
> +	.write = pci_generic_config_write,


Can you change these with pci_generic_config_read32 and 
pci_generic_config_write32? We have some targets that can only do 32 
bits PCI config space access.

> +};
> +
> +#ifdef CONFIG_PCI_MMCONFIG
> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
>   {
> -	return -ENXIO;
> +	struct pci_mmcfg_region *cfg;
> +	struct acpi_pci_root *root;
> +	int seg, start, end, err;
> +
> +	root = ci->root;
> +	seg = root->segment;
> +	start = root->secondary.start;
> +	end = root->secondary.end;
> +
> +	cfg = pci_mmconfig_lookup(seg, start);
> +	if (cfg)
> +		return 0;
> +
> +	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
> +	if (!cfg)
> +		return -ENOMEM;
> +
> +	err = pci_mmconfig_inject(cfg);
> +	return err;
>   }
>
> -#ifdef CONFIG_ACPI
> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
> +{
> +	struct acpi_pci_root *root = ci->root;
> +	struct pci_mmcfg_region *cfg;
> +
> +	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
> +	if (cfg)
> +		return;
> +
> +	if (cfg->hot_added)
> +		pci_mmconfig_delete(root->segment, root->secondary.start,
> +				    root->secondary.end);
> +}
> +#else
> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> +{
> +	return 0;
> +}
> +
> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
> +#endif
> +
> +static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
> +{
> +	return pci_add_mmconfig_region(ci);
> +}
> +
> +static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
> +{
> +	pci_remove_mmconfig_region(ci);
> +	kfree(ci);
> +}
> +
> +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> +{
> +	struct resource_entry *entry, *tmp;
> +	int ret;
> +
> +	ret = acpi_pci_probe_root_resources(ci);
> +	if (ret < 0)
> +		return ret;
> +
> +	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
> +		struct resource *res = entry->res;
> +
> +		/*
> +		 * Special handling for ARM IO range
> +		 * 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;
> +
> +			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 - 1;
> +
> +			if (pci_remap_iospace(res, res->start) < 0)
> +				resource_list_destroy_entry(entry);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static struct acpi_pci_root_ops acpi_pci_root_ops = {
> +	.pci_ops = &pci_root_ops,
> +	.init_info = pci_acpi_root_init_info,
> +	.release_info = pci_acpi_root_release_info,
> +	.prepare_resources = pci_acpi_root_prepare_resources,
> +};
> +
>   /* Root bridge scanning */
>   struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>   {
> -	/* TODO: Should be revisited when implementing PCI on ACPI */
> -	return NULL;
> +	int node = acpi_get_node(root->device->handle);
> +	int domain = root->segment;
> +	int busnum = root->secondary.start;
> +	struct acpi_pci_root_info *info;
> +	struct pci_bus *bus;
> +
> +	if (domain && !pci_domains_supported) {
> +		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
> +			domain, busnum);
> +		return NULL;
> +	}
> +
> +	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
> +	if (!info) {
> +		dev_err(&root->device->dev,
> +			"pci_bus %04x:%02x: ignored (out of memory)\n",
> +			domain, busnum);
> +		return NULL;
> +	}
> +
> +	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
> +
> +	/* After the PCI-E bus has been walked and all devices discovered,
> +	 * configure any settings of the fabric that might be necessary.
> +	 */
> +	if (bus) {
> +		struct pci_bus *child;
> +
> +		list_for_each_entry(child, &bus->children, node)
> +			pcie_bus_configure_settings(child);
> +	}
> +
> +	return bus;
>   }
>   #endif
>

Sinan Kaya Oct. 28, 2015, 8:36 p.m. UTC | #5

On 10/28/2015 2:46 PM, Sinan Kaya wrote:
>
> On 10/27/2015 12:38 PM, Tomasz Nowicki wrote:
>> Because of two patch series:
>> 1. Jiang Liu's common interface to support PCI host bridge init
>> 2. Refactoring of MMCONFIG, part of this patch set
>> now we can think about PCI buses enumeration for ARM64 and ACPI tables.
>>
>> This patch introduce ACPI based PCI hostbridge init calls which
>> use information from MCFG table (PCI config space regions) and
>> _CRS (IO/irq resources) to initialize PCI hostbridge.
>>
>> Signed-off-by: Tomasz Nowicki <[e-mail]>
>> Signed-off-by: Hanjun Guo <[e-mail]>
>> Signed-off-by: Suravee Suthikulpanit <[e-mail]>
>> CC: Arnd Bergmann <[e-mail]>
>> CC: Catalin Marinas <[e-mail]>
>> CC: Liviu Dudau <[e-mail]>
>> CC: Lorenzo Pieralisi <[e-mail]>
>> CC: Will Deacon <[e-mail]>
>> ---
>>   arch/arm64/Kconfig      |   6 ++
>>   arch/arm64/kernel/pci.c | 208
>> +++++++++++++++++++++++++++++++++++++++++++++---
>>   2 files changed, 202 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 07d1811..bbcc6b1 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -89,6 +89,7 @@ config ARM64
>>       select SPARSE_IRQ
>>       select SYSCTL_EXCEPTION_TRACE
>>       select HAVE_CONTEXT_TRACKING
>> +    select HAVE_PCI_ECAM
>>       help
>>         ARM 64-bit (AArch64) Linux support.
>>
>> @@ -202,6 +203,11 @@ source "drivers/pci/Kconfig"
>>   source "drivers/pci/pcie/Kconfig"
>>   source "drivers/pci/hotplug/Kconfig"
>>
>> +config PCI_MMCONFIG
>> +    def_bool y
>> +    select PCI_ECAM
>> +    depends on ACPI
>> +
>>   endmenu
>>
>>   menu "Kernel Features"
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index b3d098b..66cc1ae 100644
>> --- a/arch/arm64/kernel/pci.c
>> +++ b/arch/arm64/kernel/pci.c
>> @@ -11,12 +11,15 @@
>>    */
>>
>>   #include <linux/acpi.h>
>> +#include <linux/ecam.h>
>>   #include <linux/init.h>
>>   #include <linux/io.h>
>>   #include <linux/kernel.h>
>>   #include <linux/mm.h>
>> +#include <linux/of_address.h>
>>   #include <linux/of_pci.h>
>>   #include <linux/of_platform.h>
>> +#include <linux/pci-acpi.h>
>>   #include <linux/slab.h>
>>
>>   #include <asm/pci-bridge.h>
>> @@ -52,35 +55,216 @@ int pcibios_enable_device(struct pci_dev *dev,
>> int mask)
>>   }
>>
>>   /*
>> - * Try to assign the IRQ number from DT when adding a new device
>> + * Try to assign the IRQ number from DT/ACPI when adding a new device
>>    */
>>   int pcibios_add_device(struct pci_dev *dev)
>>   {
>> -    dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
>> +    if (acpi_disabled)
>> +        dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
>> +#ifdef CONFIG_ACPI
>> +    else
>> +        acpi_pci_irq_enable(dev);
>> +#endif
>>
>>       return 0;
>>   }
>>
>> +#ifdef CONFIG_ACPI
>> +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
>> +{
>> +    struct acpi_pci_root *root = bridge->bus->sysdata;
>> +
>> +    ACPI_COMPANION_SET(&bridge->dev, root->device);
>> +    return 0;
>> +}
>> +
>> +void pcibios_add_bus(struct pci_bus *bus)
>> +{
>> +    acpi_pci_add_bus(bus);
>> +}
>> +
>> +void pcibios_remove_bus(struct pci_bus *bus)
>> +{
>> +    acpi_pci_remove_bus(bus);
>> +}
>> +
>> +static int __init pcibios_assign_resources(void)
>> +{
>> +    if (acpi_disabled)
>> +        return 0;
>> +
>> +    pci_assign_unassigned_resources();
>> +    return 0;
>> +}
>>   /*
>> - * raw_pci_read/write - Platform-specific PCI config space access.
>> + * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
>> + * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
>>    */
>> -int raw_pci_read(unsigned int domain, unsigned int bus,
>> -          unsigned int devfn, int reg, int len, u32 *val)
>> +rootfs_initcall(pcibios_assign_resources);
>> +
>> +static void __iomem *
>> +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
>>   {
>> -    return -ENXIO;
>> +    struct pci_mmcfg_region *cfg;
>> +
>> +    cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
>> +    if (cfg && cfg->virt)
>> +        return cfg->virt +
>> +            (PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
>> +            offset;
>> +    return NULL;
>>   }
>>
>> -int raw_pci_write(unsigned int domain, unsigned int bus,
>> -        unsigned int devfn, int reg, int len, u32 val)
>> +struct pci_ops pci_root_ops = {
>> +    .map_bus = pci_mcfg_dev_base,
>> +    .read = pci_generic_config_read,
>> +    .write = pci_generic_config_write,
>
>
> Can you change these with pci_generic_config_read32 and
> pci_generic_config_write32? We have some targets that can only do 32
> bits PCI config space access.
>
>> +};
>> +
>> +#ifdef CONFIG_PCI_MMCONFIG
>> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
>>   {
>> -    return -ENXIO;
>> +    struct pci_mmcfg_region *cfg;
>> +    struct acpi_pci_root *root;
>> +    int seg, start, end, err;
>> +
>> +    root = ci->root;
>> +    seg = root->segment;
>> +    start = root->secondary.start;
>> +    end = root->secondary.end;
>> +
>> +    cfg = pci_mmconfig_lookup(seg, start);
>> +    if (cfg)
>> +        return 0;
>> +
>> +    cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
>> +    if (!cfg)
>> +        return -ENOMEM;
>> +
>> +    err = pci_mmconfig_inject(cfg);
>> +    return err;
>>   }
>>
>> -#ifdef CONFIG_ACPI
>> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
>> +{
>> +    struct acpi_pci_root *root = ci->root;
>> +    struct pci_mmcfg_region *cfg;
>> +
>> +    cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
>> +    if (cfg)
>> +        return;
>> +
>> +    if (cfg->hot_added)
>> +        pci_mmconfig_delete(root->segment, root->secondary.start,
>> +                    root->secondary.end);
>> +}
>> +#else
>> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
>> +{
>> +    return 0;
>> +}
>> +
>> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
>> { }
>> +#endif
>> +
>> +static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
>> +{
>> +    return pci_add_mmconfig_region(ci);
>> +}
>> +
>> +static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
>> +{
>> +    pci_remove_mmconfig_region(ci);
>> +    kfree(ci);
>> +}
>> +
>> +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info
>> *ci)
>> +{
>> +    struct resource_entry *entry, *tmp;
>> +    int ret;
>> +
>> +    ret = acpi_pci_probe_root_resources(ci);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
>> +        struct resource *res = entry->res;
>> +
>> +        /*
>> +         * Special handling for ARM IO range
>> +         * 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;
>> +
>> +            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 - 1;
>> +
>> +            if (pci_remap_iospace(res, res->start) < 0)
>> +                resource_list_destroy_entry(entry);
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static struct acpi_pci_root_ops acpi_pci_root_ops = {
>> +    .pci_ops = &pci_root_ops,
>> +    .init_info = pci_acpi_root_init_info,
>> +    .release_info = pci_acpi_root_release_info,
>> +    .prepare_resources = pci_acpi_root_prepare_resources,
>> +};
>> +
>>   /* Root bridge scanning */
>>   struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>>   {
>> -    /* TODO: Should be revisited when implementing PCI on ACPI */
>> -    return NULL;
>> +    int node = acpi_get_node(root->device->handle);
>> +    int domain = root->segment;
>> +    int busnum = root->secondary.start;
>> +    struct acpi_pci_root_info *info;
>> +    struct pci_bus *bus;
>> +
>> +    if (domain && !pci_domains_supported) {
>> +        pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
>> +            domain, busnum);
>> +        return NULL;
>> +    }
>> +
>> +    info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
>> +    if (!info) {
>> +        dev_err(&root->device->dev,
>> +            "pci_bus %04x:%02x: ignored (out of memory)\n",
>> +            domain, busnum);
>> +        return NULL;
>> +    }
>> +
>> +    bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
>> +
>> +    /* After the PCI-E bus has been walked and all devices discovered,
>> +     * configure any settings of the fabric that might be necessary.
>> +     */
>> +    if (bus) {
>> +        struct pci_bus *child;
>> +
>> +        list_for_each_entry(child, &bus->children, node)
>> +            pcie_bus_configure_settings(child);
>> +    }
>> +
>> +    return bus;
>>   }
>>   #endif
>>
>

Tomasz,
I was asked to test your new patchset on our platform. With the new 
patchset, I'm seeing two problems.

1. ACPI code is unable to discover the interrupt numbers when objects 
are ordered as follows in the ACPI file

PNP0A08 object
PNP0C0F INTA object
PNP0C0F INTB object
PNP0C0F INTC object
PNP0C0F INTD object

This gives me invalid link context error.

pci 0000:00:00.0: PCI INT A: no GSI
pci 0000:01:00.0: Derived GSI for 0000:01:00.0 INT A from 0000:00:00.0
acpi PNP0C0F:00: Invalid link context

If I order it like this in the ACPI file,

PNP0C0F INTA object
PNP0C0F INTB object
PNP0C0F INTC object
PNP0C0F INTD object
PNP0A08 object

then, the legacy interrupt numbers can be discovered properly.

2. The second problem is about the PCIe resources.

pci_0000:01:00.0:_can't_enable_device:_BAR_0_[mem_0x80100100000-0x80100101fff_64bit]_not_claimed
pci 0000:01:00.0: Can't enable PCI device, BIOS handoff failed.
pci 0000:00:00.0: BAR 14: assigned [mem 0x80100100000-0x801003fffff]
pci 0000:00:00.0: BAR 13: no space for [io  size 0x1000]
pci 0000:00:00.0: BAR 13: failed to assign [io  size 0x1000]
pci 0000:00:00.0: BAR 13: no space for [io  size 0x1000]
pci 0000:00:00.0: BAR 13: failed to assign [io  size 0x1000]
pci 0000:01:00.0: BAR 0: assigned [mem 0x80100100000-0x80100101fff 64bit]

For some reason, the kernel is unable to assign resources.

I appreciate any pointers you might give.

Tomasz Nowicki Oct. 29, 2015, 11:38 a.m. UTC | #6

On 28.10.2015 21:36, Sinan Kaya wrote:
> 1. ACPI code is unable to discover the interrupt numbers when objects
> are ordered as follows in the ACPI file
>
> PNP0A08 object
> PNP0C0F INTA object
> PNP0C0F INTB object
> PNP0C0F INTC object
> PNP0C0F INTD object
>
> This gives me invalid link context error.
>
> pci 0000:00:00.0: PCI INT A: no GSI
> pci 0000:01:00.0: Derived GSI for 0000:01:00.0 INT A from 0000:00:00.0
> acpi PNP0C0F:00: Invalid link context
>
> If I order it like this in the ACPI file,
>
> PNP0C0F INTA object
> PNP0C0F INTB object
> PNP0C0F INTC object
> PNP0C0F INTD object
> PNP0A08 object
>
> then, the legacy interrupt numbers can be discovered properly.

Can you show full content of your PNP0C0F and PNP0A08 objects?

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

Sinan Kaya Oct. 29, 2015, 2:57 p.m. UTC | #7

On 10/28/2015 4:36 PM, Sinan Kaya wrote:
> 2. The second problem is about the PCIe resources.
>
> pci_0000:01:00.0:_can't_enable_device:_BAR_0_[mem_0x80100100000-0x80100101fff_64bit]_not_claimed
>
> pci 0000:01:00.0: Can't enable PCI device, BIOS handoff failed.
> pci 0000:00:00.0: BAR 14: assigned [mem 0x80100100000-0x801003fffff]
> pci 0000:00:00.0: BAR 13: no space for [io  size 0x1000]
> pci 0000:00:00.0: BAR 13: failed to assign [io  size 0x1000]
> pci 0000:00:00.0: BAR 13: no space for [io  size 0x1000]
> pci 0000:00:00.0: BAR 13: failed to assign [io  size 0x1000]
> pci 0000:01:00.0: BAR 0: assigned [mem 0x80100100000-0x80100101fff 64bit]
>
> For some reason, the kernel is unable to assign resources.
>
> I appreciate any pointers you might give.

Tomasz,
I debugged this part of the problem yesterday night.

I have one set up with 4.2 kernel and Mark Salter's original ACPI PCIE 
patch. His patchwork works fine with our ACPI table.

This is what IO resource looks like in our table. PCI IO is supported 
using the ACPI translation attribute on ARM systems.

QWORDIO(			// produced resource
	ResourceProducer,	// bit 0 of general flags is 0
	MinFixed,		// Range is fixed
	MaxFixed,		// Range is fixed
	PosDecode,
	EntireRange,
	0x0000,			// Granularity
	0x1000,			// Min,
	0xFFFF,			// Max
	0x8FFFFFEF000,		// Translation
	0xF000,			// Range Length
	,, PI00
  )

It looks like you are missing the translation support for the IO 
aperture and Mark Salter's patch has this support.

Yours is missing in pci_acpi_root_prepare_resources function here. 
Mark's patch uses the offset argument to reorganize the resource.


+            resource_size_t length = res->end - 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 - 1;
+
+            if (pci_remap_iospace(res, res->start) < 0)
+                resource_list_destroy_entry(entry);



 > 
pci_0000:01:00.0:_can't_enable_device:_BAR_0_[mem_0x80100100000-0x80100101fff_64bit]_not_claimed 

 >
 > pci 0000:01:00.0: Can't enable PCI device, BIOS handoff failed.

I also found out this problem. This is a resource reclaim order problem 
with respect to quirks. The card I'm testing is a PCIe USB card. It has 
a quirk associated with it and it tries to enable and then disable the 
device here.

DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
PCI_CLASS_SERIAL_USB, 8, quirk_usb_early_handoff);

The resources get assigned after this quirk. That's why I see these 
can't enable messages. I looked at 4.2 kernel. The execution order is 
reversed.

 > pci 0000:00:00.0: BAR 14: assigned [mem 0x80100100000-0x801003fffff]
 > pci 0000:00:00.0: BAR 13: no space for [io  size 0x1000]
 > pci 0000:00:00.0: BAR 13: failed to assign [io  size 0x1000]
 > pci 0000:00:00.0: BAR 13: no space for [io  size 0x1000]
 > pci 0000:00:00.0: BAR 13: failed to assign [io  size 0x1000]
 > pci 0000:01:00.0: BAR 0: assigned [mem 0x80100100000-0x80100101fff 64bit]

Sinan Kaya Oct. 29, 2015, 3:01 p.m. UTC | #8

On 10/29/2015 7:38 AM, Tomasz Nowicki wrote:
> On 28.10.2015 21:36, Sinan Kaya wrote:
>> 1. ACPI code is unable to discover the interrupt numbers when objects
>> are ordered as follows in the ACPI file
>>
>> PNP0A08 object
>> PNP0C0F INTA object
>> PNP0C0F INTB object
>> PNP0C0F INTC object
>> PNP0C0F INTD object
>>
>> This gives me invalid link context error.
>>
>> pci 0000:00:00.0: PCI INT A: no GSI
>> pci 0000:01:00.0: Derived GSI for 0000:01:00.0 INT A from 0000:00:00.0
>> acpi PNP0C0F:00: Invalid link context
>>
>> If I order it like this in the ACPI file,
>>
>> PNP0C0F INTA object
>> PNP0C0F INTB object
>> PNP0C0F INTC object
>> PNP0C0F INTD object
>> PNP0A08 object
>>
>> then, the legacy interrupt numbers can be discovered properly.
>
> Can you show full content of your PNP0C0F and PNP0A08 objects?
>

ACPI table is considered proprietary. I don't think I can get the legal 
approval in time. I can give you pieces though.

Here is the _PRT
Device (PCI0) { // PCIe port 0
	Name(_HID, EISAID("PNP0A08"))	// PCI express
	Name(_CID, EISAID("PNP0A03"))	// Compatible PCI Root Bridge
{
	....
	Name(_PRT, Package(){
		Package(){0x0FFFF, 0, \_SB.LN0A, 0},  // Slot 0, INTA
		Package(){0x0FFFF, 1, \_SB.LN0B, 0},  // Slot 0, INTB
		Package(){0x0FFFF, 2, \_SB.LN0C, 0},  // Slot 0, INTC
		Package(){0x0FFFF, 3, \_SB.LN0D, 0}   // Slot 0, INTD
	})
}

Here is the PNP0C0F

Device(LN0A){
	Name(_HID, EISAID("PNP0C0F")) // PCI interrupt link
	Name(_UID, 1)
	Name(_PRS, ResourceTemplate(){
	Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive, , ,) {0xE8}
	})
	Method(_DIS) {}
	Method(_CRS) { Return (_PRS) }
	Method(_SRS, 1) {}
}



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

Tomasz Nowicki Oct. 29, 2015, 4:27 p.m. UTC | #9

On 29.10.2015 15:57, Sinan Kaya wrote:
>
>
> On 10/28/2015 4:36 PM, Sinan Kaya wrote:
>> 2. The second problem is about the PCIe resources.
>>
>> pci_0000:01:00.0:_can't_enable_device:_BAR_0_[mem_0x80100100000-0x80100101fff_64bit]_not_claimed
>>
>>
>> pci 0000:01:00.0: Can't enable PCI device, BIOS handoff failed.
>> pci 0000:00:00.0: BAR 14: assigned [mem 0x80100100000-0x801003fffff]
>> pci 0000:00:00.0: BAR 13: no space for [io  size 0x1000]
>> pci 0000:00:00.0: BAR 13: failed to assign [io  size 0x1000]
>> pci 0000:00:00.0: BAR 13: no space for [io  size 0x1000]
>> pci 0000:00:00.0: BAR 13: failed to assign [io  size 0x1000]
>> pci 0000:01:00.0: BAR 0: assigned [mem 0x80100100000-0x80100101fff 64bit]
>>
>> For some reason, the kernel is unable to assign resources.
>>
>> I appreciate any pointers you might give.
>
> Tomasz,
> I debugged this part of the problem yesterday night.
>
> I have one set up with 4.2 kernel and Mark Salter's original ACPI PCIE
> patch. His patchwork works fine with our ACPI table.
>
> This is what IO resource looks like in our table. PCI IO is supported
> using the ACPI translation attribute on ARM systems.
>
> QWORDIO(            // produced resource
>      ResourceProducer,    // bit 0 of general flags is 0
>      MinFixed,        // Range is fixed
>      MaxFixed,        // Range is fixed
>      PosDecode,
>      EntireRange,
>      0x0000,            // Granularity
>      0x1000,            // Min,
>      0xFFFF,            // Max
>      0x8FFFFFEF000,        // Translation
>      0xF000,            // Range Length
>      ,, PI00
>   )
>
> It looks like you are missing the translation support for the IO
> aperture and Mark Salter's patch has this support.
>
> Yours is missing in pci_acpi_root_prepare_resources function here.
> Mark's patch uses the offset argument to reorganize the resource.
>
>
> +            resource_size_t length = res->end - 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 - 1;
> +
> +            if (pci_remap_iospace(res, res->start) < 0)
> +                resource_list_destroy_entry(entry);
>
>

Thanks for the heads up, working on it.

Regards,
Tomasz


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

Lorenzo Pieralisi Nov. 3, 2015, 2:15 p.m. UTC | #10

On Wed, Oct 28, 2015 at 02:46:37PM -0400, Sinan Kaya wrote:

[...]

> >-int raw_pci_write(unsigned int domain, unsigned int bus,
> >-		unsigned int devfn, int reg, int len, u32 val)
> >+struct pci_ops pci_root_ops = {
> >+	.map_bus = pci_mcfg_dev_base,
> >+	.read = pci_generic_config_read,
> >+	.write = pci_generic_config_write,
> 
> 
> Can you change these with pci_generic_config_read32 and
> pci_generic_config_write32? We have some targets that can only do 32
> bits PCI config space access.

No.

http://www.spinics.net/lists/linux-pci/msg44869.html

Can you be a bit more specific please ?

Sigh. Looks like we have to start adding platform specific quirks even
before we merged the generic ACPI PCIe host controller implementation.

Lorenzo

> >+};
> >+
> >+#ifdef CONFIG_PCI_MMCONFIG
> >+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> >  {
> >-	return -ENXIO;
> >+	struct pci_mmcfg_region *cfg;
> >+	struct acpi_pci_root *root;
> >+	int seg, start, end, err;
> >+
> >+	root = ci->root;
> >+	seg = root->segment;
> >+	start = root->secondary.start;
> >+	end = root->secondary.end;
> >+
> >+	cfg = pci_mmconfig_lookup(seg, start);
> >+	if (cfg)
> >+		return 0;
> >+
> >+	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
> >+	if (!cfg)
> >+		return -ENOMEM;
> >+
> >+	err = pci_mmconfig_inject(cfg);
> >+	return err;
> >  }
> >
> >-#ifdef CONFIG_ACPI
> >+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
> >+{
> >+	struct acpi_pci_root *root = ci->root;
> >+	struct pci_mmcfg_region *cfg;
> >+
> >+	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
> >+	if (cfg)
> >+		return;
> >+
> >+	if (cfg->hot_added)
> >+		pci_mmconfig_delete(root->segment, root->secondary.start,
> >+				    root->secondary.end);
> >+}
> >+#else
> >+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> >+{
> >+	return 0;
> >+}
> >+
> >+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
> >+#endif
> >+
> >+static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
> >+{
> >+	return pci_add_mmconfig_region(ci);
> >+}
> >+
> >+static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
> >+{
> >+	pci_remove_mmconfig_region(ci);
> >+	kfree(ci);
> >+}
> >+
> >+static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> >+{
> >+	struct resource_entry *entry, *tmp;
> >+	int ret;
> >+
> >+	ret = acpi_pci_probe_root_resources(ci);
> >+	if (ret < 0)
> >+		return ret;
> >+
> >+	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
> >+		struct resource *res = entry->res;
> >+
> >+		/*
> >+		 * Special handling for ARM IO range
> >+		 * 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;
> >+
> >+			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 - 1;
> >+
> >+			if (pci_remap_iospace(res, res->start) < 0)
> >+				resource_list_destroy_entry(entry);
> >+		}
> >+	}
> >+
> >+	return 0;
> >+}
> >+
> >+static struct acpi_pci_root_ops acpi_pci_root_ops = {
> >+	.pci_ops = &pci_root_ops,
> >+	.init_info = pci_acpi_root_init_info,
> >+	.release_info = pci_acpi_root_release_info,
> >+	.prepare_resources = pci_acpi_root_prepare_resources,
> >+};
> >+
> >  /* Root bridge scanning */
> >  struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> >  {
> >-	/* TODO: Should be revisited when implementing PCI on ACPI */
> >-	return NULL;
> >+	int node = acpi_get_node(root->device->handle);
> >+	int domain = root->segment;
> >+	int busnum = root->secondary.start;
> >+	struct acpi_pci_root_info *info;
> >+	struct pci_bus *bus;
> >+
> >+	if (domain && !pci_domains_supported) {
> >+		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
> >+			domain, busnum);
> >+		return NULL;
> >+	}
> >+
> >+	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
> >+	if (!info) {
> >+		dev_err(&root->device->dev,
> >+			"pci_bus %04x:%02x: ignored (out of memory)\n",
> >+			domain, busnum);
> >+		return NULL;
> >+	}
> >+
> >+	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
> >+
> >+	/* After the PCI-E bus has been walked and all devices discovered,
> >+	 * configure any settings of the fabric that might be necessary.
> >+	 */
> >+	if (bus) {
> >+		struct pci_bus *child;
> >+
> >+		list_for_each_entry(child, &bus->children, node)
> >+			pcie_bus_configure_settings(child);
> >+	}
> >+
> >+	return bus;
> >  }
> >  #endif
> >
> 
> -- 
> Sinan Kaya
> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> Linux Foundation Collaborative Project
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lorenzo Pieralisi Nov. 3, 2015, 2:32 p.m. UTC | #11

On Wed, Oct 28, 2015 at 11:49:40AM +0000, [e-mail] wrote:
> On Tue, Oct 27, 2015 at 05:38:42PM +0100, Tomasz Nowicki wrote:

[...]

> > +static int __init pcibios_assign_resources(void)
> > +{
> > +	if (acpi_disabled)
> > +		return 0;
> > +
> > +	pci_assign_unassigned_resources();
> > +	return 0;
> 
> You can change this function into:
> {
> 	if (!acpi_disabled)
> 		pci_assign_unassigned_resources();
> 
> 	return 0;
> }
> 
> as the equivalent but shorter form.

I do not think it is a matter of code style here, it is a matter
of understanding when and if we want to reassign resources on ACPI
systems, it is an open question on ARM64 that must be sorted out (ie we
ignore FW/BARs set-up entirely).

> > +}
> >  /*
> > - * raw_pci_read/write - Platform-specific PCI config space access.
> > + * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
> > + * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
> >   */
> > -int raw_pci_read(unsigned int domain, unsigned int bus,
> > -		  unsigned int devfn, int reg, int len, u32 *val)
> 
> What happened with raw_pci_{read,write} ? Why do you remove them?
> 
> 
> > +rootfs_initcall(pcibios_assign_resources);
> 
> Would you be so kind and explain to me why you need this initcall?
> Can you not set the PCI_REASSIGN_ALL_RSRC flag before calling
> pci_scan_root_bus() ?

On what basis ? BTW, PCI core code does not assign unassigned resources
anyway even if that flag is set, so some policy has to be defined here.

Thanks,
Lorenzo

> I haven't focused on ACPI before so I'm a bit hazy on the init order when
> that is enabled. That being said, I don't like adding in the architecture
> code initcall hooks just to fix up some dependency orders that could / should
> be fixed some other way.
> 
> > +
> > +static void __iomem *
> > +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
> >  {
> > -	return -ENXIO;
> > +	struct pci_mmcfg_region *cfg;
> > +
> > +	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
> > +	if (cfg && cfg->virt)
> > +		return cfg->virt +
> > +			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
> > +			offset;
> > +	return NULL;
> >  }
> >  
> > -int raw_pci_write(unsigned int domain, unsigned int bus,
> > -		unsigned int devfn, int reg, int len, u32 val)
> > +struct pci_ops pci_root_ops = {
> > +	.map_bus = pci_mcfg_dev_base,
> > +	.read = pci_generic_config_read,
> > +	.write = pci_generic_config_write,
> > +};
> > +
> > +#ifdef CONFIG_PCI_MMCONFIG
> > +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> >  {
> > -	return -ENXIO;
> > +	struct pci_mmcfg_region *cfg;
> > +	struct acpi_pci_root *root;
> > +	int seg, start, end, err;
> > +
> > +	root = ci->root;
> > +	seg = root->segment;
> > +	start = root->secondary.start;
> > +	end = root->secondary.end;
> > +
> > +	cfg = pci_mmconfig_lookup(seg, start);
> > +	if (cfg)
> > +		return 0;
> > +
> > +	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
> > +	if (!cfg)
> > +		return -ENOMEM;
> > +
> > +	err = pci_mmconfig_inject(cfg);
> > +	return err;
> >  }
> >  
> > -#ifdef CONFIG_ACPI
> > +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
> > +{
> > +	struct acpi_pci_root *root = ci->root;
> > +	struct pci_mmcfg_region *cfg;
> > +
> > +	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
> > +	if (cfg)
> > +		return;
> > +
> > +	if (cfg->hot_added)
> > +		pci_mmconfig_delete(root->segment, root->secondary.start,
> > +				    root->secondary.end);
> > +}
> > +#else
> > +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> > +{
> > +	return 0;
> > +}
> > +
> > +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
> > +#endif
> > +
> > +static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
> > +{
> > +	return pci_add_mmconfig_region(ci);
> > +}
> > +
> > +static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
> > +{
> > +	pci_remove_mmconfig_region(ci);
> > +	kfree(ci);
> > +}
> > +
> > +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> > +{
> > +	struct resource_entry *entry, *tmp;
> > +	int ret;
> > +
> > +	ret = acpi_pci_probe_root_resources(ci);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
> > +		struct resource *res = entry->res;
> > +
> > +		/*
> > +		 * Special handling for ARM IO range
> 
> There is nothing ARM specific here. It should apply to any memory mapped IO range.
> 
> > +		 * 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;
> > +
> > +			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 - 1;
> > +
> > +			if (pci_remap_iospace(res, res->start) < 0)
> > +				resource_list_destroy_entry(entry);
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct acpi_pci_root_ops acpi_pci_root_ops = {
> > +	.pci_ops = &pci_root_ops,
> > +	.init_info = pci_acpi_root_init_info,
> > +	.release_info = pci_acpi_root_release_info,
> > +	.prepare_resources = pci_acpi_root_prepare_resources,
> > +};
> > +
> >  /* Root bridge scanning */
> >  struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> >  {
> > -	/* TODO: Should be revisited when implementing PCI on ACPI */
> > -	return NULL;
> > +	int node = acpi_get_node(root->device->handle);
> > +	int domain = root->segment;
> > +	int busnum = root->secondary.start;
> > +	struct acpi_pci_root_info *info;
> > +	struct pci_bus *bus;
> > +
> > +	if (domain && !pci_domains_supported) {
> > +		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
> > +			domain, busnum);
> > +		return NULL;
> > +	}
> > +
> > +	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
> > +	if (!info) {
> > +		dev_err(&root->device->dev,
> > +			"pci_bus %04x:%02x: ignored (out of memory)\n",
> > +			domain, busnum);
> > +		return NULL;
> > +	}
> > +
> > +	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
> > +
> > +	/* After the PCI-E bus has been walked and all devices discovered,
> > +	 * configure any settings of the fabric that might be necessary.
> > +	 */
> > +	if (bus) {
> > +		struct pci_bus *child;
> > +
> > +		list_for_each_entry(child, &bus->children, node)
> > +			pcie_bus_configure_settings(child);
> > +	}
> > +
> > +	return bus;
> >  }
> >  #endif
> > -- 
> > 1.9.1
> > 
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(?)_/¯
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tomasz Nowicki Nov. 3, 2015, 2:39 p.m. UTC | #12

On 03.11.2015 15:15, Lorenzo Pieralisi wrote:
> On Wed, Oct 28, 2015 at 02:46:37PM -0400, Sinan Kaya wrote:
>
> [...]
>
>>> -int raw_pci_write(unsigned int domain, unsigned int bus,
>>> -		unsigned int devfn, int reg, int len, u32 val)
>>> +struct pci_ops pci_root_ops = {
>>> +	.map_bus = pci_mcfg_dev_base,
>>> +	.read = pci_generic_config_read,
>>> +	.write = pci_generic_config_write,
>>
>>
>> Can you change these with pci_generic_config_read32 and
>> pci_generic_config_write32? We have some targets that can only do 32
>> bits PCI config space access.
>
> No.
>
> http://www.spinics.net/lists/linux-pci/msg44869.html
>
> Can you be a bit more specific please ?
>
> Sigh. Looks like we have to start adding platform specific quirks even
> before we merged the generic ACPI PCIe host controller implementation.
>

The sad reality... But my next version will be still generic. Once that 
one appear to be in good shape then we can add quirks.

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

Sinan Kaya Nov. 3, 2015, 3:10 p.m. UTC | #13

On 11/3/2015 9:39 AM, Tomasz Nowicki wrote:
>>>> +struct pci_ops pci_root_ops = {
>>>> +    .map_bus = pci_mcfg_dev_base,
>>>> +    .read = pci_generic_config_read,
>>>> +    .write = pci_generic_config_write,
>>>
>>>
>>> Can you change these with pci_generic_config_read32 and
>>> pci_generic_config_write32? We have some targets that can only do 32
>>> bits PCI config space access.
>>
>> No.
>>
>> http://www.spinics.net/lists/linux-pci/msg44869.html
>>
>> Can you be a bit more specific please ?
>>
>> Sigh. Looks like we have to start adding platform specific quirks even
>> before we merged the generic ACPI PCIe host controller implementation.
>>
>
> The sad reality... But my next version will be still generic. Once that
> one appear to be in good shape then we can add quirks.

Thanks.

I don't see anywhere in the SBSA spec addendum that the PCI 
configuration space section that unaligned accesses *MUST* be supported.

If this is required, please have this info added to the spec. I can work 
with the designers for the next chip.

Unaligned access on the current hardware returns incomplete values or 
can cause bus faults. The behavior is undefined.

Hanjun Guo Nov. 3, 2015, 3:19 p.m. UTC | #14

On 11/03/2015 10:15 PM, Lorenzo Pieralisi wrote:
> On Wed, Oct 28, 2015 at 02:46:37PM -0400, Sinan Kaya wrote:
>
> [...]
>
>>> -int raw_pci_write(unsigned int domain, unsigned int bus,
>>> -		unsigned int devfn, int reg, int len, u32 val)
>>> +struct pci_ops pci_root_ops = {
>>> +	.map_bus = pci_mcfg_dev_base,
>>> +	.read = pci_generic_config_read,
>>> +	.write = pci_generic_config_write,
>>
>>
>> Can you change these with pci_generic_config_read32 and
>> pci_generic_config_write32? We have some targets that can only do 32
>> bits PCI config space access.
>
> No.
>
> http://www.spinics.net/lists/linux-pci/msg44869.html
>
> Can you be a bit more specific please ?
>
> Sigh. Looks like we have to start adding platform specific quirks even
> before we merged the generic ACPI PCIe host controller implementation.

Cc Gab, Zhou, and Dondong who upstream the hip05 (designware) PCIe host
support.

I think so, some platform may not support ECAM for root complex,
which needs special handling of access config space, we may need
to consider those cases.

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

Arnd Bergmann Nov. 3, 2015, 3:59 p.m. UTC | #15

On Tuesday 03 November 2015 10:10:21 Sinan Kaya wrote:
> 
> I don't see anywhere in the SBSA spec addendum that the PCI 
> configuration space section that unaligned accesses *MUST* be supported.
> 
> If this is required, please have this info added to the spec. I can work 
> with the designers for the next chip.
> 
> Unaligned access on the current hardware returns incomplete values or 
> can cause bus faults. The behavior is undefined.

Unaligned accesses are not allowed, but any PCI compliant device must
support aligned 1, 2 or 4 byte accesses on its configuration space,
though the byte-enable mechanism. In an ECAM host bridge, those are
mapped to load/store accesses from the CPU with the respective width
and natural alignment.

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

[e-mail] Nov. 3, 2015, 4:28 p.m. UTC | #16

On Tue, Nov 03, 2015 at 02:32:14PM +0000, Lorenzo Pieralisi wrote:
> On Wed, Oct 28, 2015 at 11:49:40AM +0000, [e-mail] wrote:
> > On Tue, Oct 27, 2015 at 05:38:42PM +0100, Tomasz Nowicki wrote:
> 
> [...]
> 
> > > +static int __init pcibios_assign_resources(void)
> > > +{
> > > +	if (acpi_disabled)
> > > +		return 0;
> > > +
> > > +	pci_assign_unassigned_resources();
> > > +	return 0;
> > 
> > You can change this function into:
> > {
> > 	if (!acpi_disabled)
> > 		pci_assign_unassigned_resources();
> > 
> > 	return 0;
> > }
> > 
> > as the equivalent but shorter form.
> 
> I do not think it is a matter of code style here, it is a matter
> of understanding when and if we want to reassign resources on ACPI
> systems, it is an open question on ARM64 that must be sorted out (ie we
> ignore FW/BARs set-up entirely).

I was reviewing the code here, not doing any technical guidance, I'm
leaving that to you as you are more involved around ACPI.

> 
> > > +}
> > >  /*
> > > - * raw_pci_read/write - Platform-specific PCI config space access.
> > > + * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
> > > + * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
> > >   */
> > > -int raw_pci_read(unsigned int domain, unsigned int bus,
> > > -		  unsigned int devfn, int reg, int len, u32 *val)
> > 
> > What happened with raw_pci_{read,write} ? Why do you remove them?
> > 
> > 
> > > +rootfs_initcall(pcibios_assign_resources);
> > 
> > Would you be so kind and explain to me why you need this initcall?
> > Can you not set the PCI_REASSIGN_ALL_RSRC flag before calling
> > pci_scan_root_bus() ?
> 
> On what basis ? BTW, PCI core code does not assign unassigned resources
> anyway even if that flag is set, so some policy has to be defined here.

I was thinking that ACPI code can do that if they seem to depend on the resources
being assigned during root bus scan. I was not implying that PCI core code enforces
that.

Best regards,
Liviu

> 
> Thanks,
> Lorenzo
> 
> > I haven't focused on ACPI before so I'm a bit hazy on the init order when
> > that is enabled. That being said, I don't like adding in the architecture
> > code initcall hooks just to fix up some dependency orders that could / should
> > be fixed some other way.
> > 
> > > +
> > > +static void __iomem *
> > > +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
> > >  {
> > > -	return -ENXIO;
> > > +	struct pci_mmcfg_region *cfg;
> > > +
> > > +	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
> > > +	if (cfg && cfg->virt)
> > > +		return cfg->virt +
> > > +			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
> > > +			offset;
> > > +	return NULL;
> > >  }
> > >  
> > > -int raw_pci_write(unsigned int domain, unsigned int bus,
> > > -		unsigned int devfn, int reg, int len, u32 val)
> > > +struct pci_ops pci_root_ops = {
> > > +	.map_bus = pci_mcfg_dev_base,
> > > +	.read = pci_generic_config_read,
> > > +	.write = pci_generic_config_write,
> > > +};
> > > +
> > > +#ifdef CONFIG_PCI_MMCONFIG
> > > +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> > >  {
> > > -	return -ENXIO;
> > > +	struct pci_mmcfg_region *cfg;
> > > +	struct acpi_pci_root *root;
> > > +	int seg, start, end, err;
> > > +
> > > +	root = ci->root;
> > > +	seg = root->segment;
> > > +	start = root->secondary.start;
> > > +	end = root->secondary.end;
> > > +
> > > +	cfg = pci_mmconfig_lookup(seg, start);
> > > +	if (cfg)
> > > +		return 0;
> > > +
> > > +	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
> > > +	if (!cfg)
> > > +		return -ENOMEM;
> > > +
> > > +	err = pci_mmconfig_inject(cfg);
> > > +	return err;
> > >  }
> > >  
> > > -#ifdef CONFIG_ACPI
> > > +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
> > > +{
> > > +	struct acpi_pci_root *root = ci->root;
> > > +	struct pci_mmcfg_region *cfg;
> > > +
> > > +	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
> > > +	if (cfg)
> > > +		return;
> > > +
> > > +	if (cfg->hot_added)
> > > +		pci_mmconfig_delete(root->segment, root->secondary.start,
> > > +				    root->secondary.end);
> > > +}
> > > +#else
> > > +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
> > > +#endif
> > > +
> > > +static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
> > > +{
> > > +	return pci_add_mmconfig_region(ci);
> > > +}
> > > +
> > > +static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
> > > +{
> > > +	pci_remove_mmconfig_region(ci);
> > > +	kfree(ci);
> > > +}
> > > +
> > > +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> > > +{
> > > +	struct resource_entry *entry, *tmp;
> > > +	int ret;
> > > +
> > > +	ret = acpi_pci_probe_root_resources(ci);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
> > > +		struct resource *res = entry->res;
> > > +
> > > +		/*
> > > +		 * Special handling for ARM IO range
> > 
> > There is nothing ARM specific here. It should apply to any memory mapped IO range.
> > 
> > > +		 * 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;
> > > +
> > > +			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 - 1;
> > > +
> > > +			if (pci_remap_iospace(res, res->start) < 0)
> > > +				resource_list_destroy_entry(entry);
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct acpi_pci_root_ops acpi_pci_root_ops = {
> > > +	.pci_ops = &pci_root_ops,
> > > +	.init_info = pci_acpi_root_init_info,
> > > +	.release_info = pci_acpi_root_release_info,
> > > +	.prepare_resources = pci_acpi_root_prepare_resources,
> > > +};
> > > +
> > >  /* Root bridge scanning */
> > >  struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> > >  {
> > > -	/* TODO: Should be revisited when implementing PCI on ACPI */
> > > -	return NULL;
> > > +	int node = acpi_get_node(root->device->handle);
> > > +	int domain = root->segment;
> > > +	int busnum = root->secondary.start;
> > > +	struct acpi_pci_root_info *info;
> > > +	struct pci_bus *bus;
> > > +
> > > +	if (domain && !pci_domains_supported) {
> > > +		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
> > > +			domain, busnum);
> > > +		return NULL;
> > > +	}
> > > +
> > > +	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
> > > +	if (!info) {
> > > +		dev_err(&root->device->dev,
> > > +			"pci_bus %04x:%02x: ignored (out of memory)\n",
> > > +			domain, busnum);
> > > +		return NULL;
> > > +	}
> > > +
> > > +	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
> > > +
> > > +	/* After the PCI-E bus has been walked and all devices discovered,
> > > +	 * configure any settings of the fabric that might be necessary.
> > > +	 */
> > > +	if (bus) {
> > > +		struct pci_bus *child;
> > > +
> > > +		list_for_each_entry(child, &bus->children, node)
> > > +			pcie_bus_configure_settings(child);
> > > +	}
> > > +
> > > +	return bus;
> > >  }
> > >  #endif
> > > -- 
> > > 1.9.1
> > > 
> > 
> > -- 
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(?)_/¯
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Sinan Kaya Nov. 3, 2015, 4:33 p.m. UTC | #17

On 11/3/2015 10:59 AM, Arnd Bergmann wrote:
> On Tuesday 03 November 2015 10:10:21 Sinan Kaya wrote:
>>
>> I don't see anywhere in the SBSA spec addendum that the PCI
>> configuration space section that unaligned accesses *MUST* be supported.
>>
>> If this is required, please have this info added to the spec. I can work
>> with the designers for the next chip.
>>
>> Unaligned access on the current hardware returns incomplete values or
>> can cause bus faults. The behavior is undefined.
>
> Unaligned accesses are not allowed, but any PCI compliant device must
> support aligned 1, 2 or 4 byte accesses on its configuration space,
> though the byte-enable mechanism. In an ECAM host bridge, those are
> mapped to load/store accesses from the CPU with the respective width
> and natural alignment.
>
> 	Arnd
>

As far as I see, the endpoints do not have any problems with unaligned 
accesses. It is the host bridge itself (stuff that doesn't get on the 
PCIe bus and uses traditional AXI kind bus internally) has problems with 
alignment.

If Linux is expecting all HW vendors to implement alignment support, 
this needs to be put in the SBSA spec as a hard requirement.

Arnd Bergmann Nov. 3, 2015, 4:55 p.m. UTC | #18

On Tuesday 03 November 2015 11:33:18 Sinan Kaya wrote:
> 
> On 11/3/2015 10:59 AM, Arnd Bergmann wrote:
> > On Tuesday 03 November 2015 10:10:21 Sinan Kaya wrote:
> >>
> >> I don't see anywhere in the SBSA spec addendum that the PCI
> >> configuration space section that unaligned accesses *MUST* be supported.
> >>
> >> If this is required, please have this info added to the spec. I can work
> >> with the designers for the next chip.
> >>
> >> Unaligned access on the current hardware returns incomplete values or
> >> can cause bus faults. The behavior is undefined.
> >
> > Unaligned accesses are not allowed, but any PCI compliant device must
> > support aligned 1, 2 or 4 byte accesses on its configuration space,
> > though the byte-enable mechanism. In an ECAM host bridge, those are
> > mapped to load/store accesses from the CPU with the respective width
> > and natural alignment.
> 
> As far as I see, the endpoints do not have any problems with unaligned 
> accesses. It is the host bridge itself (stuff that doesn't get on the 
> PCIe bus and uses traditional AXI kind bus internally) has problems with 
> alignment.
> 
> If Linux is expecting all HW vendors to implement alignment support, 
> this needs to be put in the SBSA spec as a hard requirement.

As I said, it's not unaligned accesses at all, just 1-byte and aligned
2-byte accesses, and it's not Linux mandating this but the PCI
spec. Please read Russell's email again, it is not possible for PCI
to work according to the specification unless the host bridge allows
sub-32-bit accesses.

You can probably work around this by using the legacy I/O port method
rather than ECAM, if the PCI host bridge itself is functional and just
the host bus it is connected to is buggy.

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

Lorenzo Pieralisi Nov. 3, 2015, 4:55 p.m. UTC | #19

On Tue, Oct 27, 2015 at 05:38:42PM +0100, Tomasz Nowicki wrote:

[...]

>  menu "Kernel Features"
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index b3d098b..66cc1ae 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -11,12 +11,15 @@
>   */
>  
>  #include <linux/acpi.h>
> +#include <linux/ecam.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/mm.h>
> +#include <linux/of_address.h>
>  #include <linux/of_pci.h>
>  #include <linux/of_platform.h>
> +#include <linux/pci-acpi.h>
>  #include <linux/slab.h>
>  
>  #include <asm/pci-bridge.h>
> @@ -52,35 +55,216 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
>  }
>  
>  /*
> - * Try to assign the IRQ number from DT when adding a new device
> + * Try to assign the IRQ number from DT/ACPI when adding a new device
>   */
>  int pcibios_add_device(struct pci_dev *dev)
>  {
> -	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> +	if (acpi_disabled)
> +		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> +#ifdef CONFIG_ACPI
> +	else
> +		acpi_pci_irq_enable(dev);
> +#endif

This series:

http://www.spinics.net/lists/linux-pci/msg45950.html

will allow us to initialize the irq mapping function according to
the boot method, code above is getting cumbersome and it is already
overriden when booting with DT, so we will remove it altogether.

>  
>  	return 0;
>  }
>  
> +#ifdef CONFIG_ACPI
> +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> +{
> +	struct acpi_pci_root *root = bridge->bus->sysdata;
> +
> +	ACPI_COMPANION_SET(&bridge->dev, root->device);
> +	return 0;

This should be made part of core code IMO.

> +}
> +
> +void pcibios_add_bus(struct pci_bus *bus)
> +{
> +	acpi_pci_add_bus(bus);
> +}
> +
> +void pcibios_remove_bus(struct pci_bus *bus)
> +{
> +	acpi_pci_remove_bus(bus);
> +}

Two functions above are identical for arm64, ia64 and x86, I do
not think they belong in arch code.

> +static int __init pcibios_assign_resources(void)
> +{
> +	if (acpi_disabled)
> +		return 0;
> +
> +	pci_assign_unassigned_resources();
> +	return 0;

Already commented on this.

> +}
>  /*
> - * raw_pci_read/write - Platform-specific PCI config space access.
> + * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
> + * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
>   */
> -int raw_pci_read(unsigned int domain, unsigned int bus,
> -		  unsigned int devfn, int reg, int len, u32 *val)
> +rootfs_initcall(pcibios_assign_resources);
> +
> +static void __iomem *
> +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
>  {
> -	return -ENXIO;
> +	struct pci_mmcfg_region *cfg;
> +
> +	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
> +	if (cfg && cfg->virt)
> +		return cfg->virt +
> +			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
> +			offset;
> +	return NULL;

Why is this code arm64 specific ?

>  }
>  
> -int raw_pci_write(unsigned int domain, unsigned int bus,
> -		unsigned int devfn, int reg, int len, u32 val)
> +struct pci_ops pci_root_ops = {
> +	.map_bus = pci_mcfg_dev_base,
> +	.read = pci_generic_config_read,
> +	.write = pci_generic_config_write,
> +};
> +
> +#ifdef CONFIG_PCI_MMCONFIG
> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
>  {
> -	return -ENXIO;
> +	struct pci_mmcfg_region *cfg;
> +	struct acpi_pci_root *root;
> +	int seg, start, end, err;
> +
> +	root = ci->root;
> +	seg = root->segment;
> +	start = root->secondary.start;
> +	end = root->secondary.end;
> +
> +	cfg = pci_mmconfig_lookup(seg, start);
> +	if (cfg)
> +		return 0;
> +
> +	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
> +	if (!cfg)
> +		return -ENOMEM;
> +
> +	err = pci_mmconfig_inject(cfg);
> +	return err;
>  }
>  
> -#ifdef CONFIG_ACPI
> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
> +{
> +	struct acpi_pci_root *root = ci->root;
> +	struct pci_mmcfg_region *cfg;
> +
> +	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
> +	if (cfg)
> +		return;
> +
> +	if (cfg->hot_added)
> +		pci_mmconfig_delete(root->segment, root->secondary.start,
> +				    root->secondary.end);
> +}
> +#else
> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> +{
> +	return 0;
> +}
> +
> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
> +#endif

Ditto.

> +
> +static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
> +{
> +	return pci_add_mmconfig_region(ci);
> +}
> +
> +static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
> +{
> +	pci_remove_mmconfig_region(ci);
> +	kfree(ci);
> +}
> +
> +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> +{
> +	struct resource_entry *entry, *tmp;
> +	int ret;
> +
> +	ret = acpi_pci_probe_root_resources(ci);
> +	if (ret < 0)
> +		return ret;

Code above is identical on arm64, ia64 and x86.

> +
> +	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
> +		struct resource *res = entry->res;
> +
> +		/*
> +		 * Special handling for ARM IO range
> +		 * 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;
> +
> +			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 - 1;
> +
> +			if (pci_remap_iospace(res, res->start) < 0)
> +				resource_list_destroy_entry(entry);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static struct acpi_pci_root_ops acpi_pci_root_ops = {
> +	.pci_ops = &pci_root_ops,
> +	.init_info = pci_acpi_root_init_info,
> +	.release_info = pci_acpi_root_release_info,
> +	.prepare_resources = pci_acpi_root_prepare_resources,
> +};
> +
>  /* Root bridge scanning */
>  struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  {
> -	/* TODO: Should be revisited when implementing PCI on ACPI */
> -	return NULL;
> +	int node = acpi_get_node(root->device->handle);
> +	int domain = root->segment;
> +	int busnum = root->secondary.start;
> +	struct acpi_pci_root_info *info;
> +	struct pci_bus *bus;
> +
> +	if (domain && !pci_domains_supported) {
> +		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
> +			domain, busnum);
> +		return NULL;
> +	}
> +
> +	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
> +	if (!info) {
> +		dev_err(&root->device->dev,
> +			"pci_bus %04x:%02x: ignored (out of memory)\n",
> +			domain, busnum);
> +		return NULL;
> +	}
> +
> +	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
> +
> +	/* After the PCI-E bus has been walked and all devices discovered,
> +	 * configure any settings of the fabric that might be necessary.
> +	 */
> +	if (bus) {
> +		struct pci_bus *child;
> +
> +		list_for_each_entry(child, &bus->children, node)
> +			pcie_bus_configure_settings(child);
> +	}
> +
> +	return bus;

Code above is entirely arch agnostic (apart from what data is passed to
sysdata) and I think there is room for further consolidation with
x86 and ia64, I will have a look into this.

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

David Daney Nov. 3, 2015, 5:39 p.m. UTC | #20

On 11/03/2015 07:19 AM, Hanjun Guo wrote:
> On 11/03/2015 10:15 PM, Lorenzo Pieralisi wrote:
>> On Wed, Oct 28, 2015 at 02:46:37PM -0400, Sinan Kaya wrote:
>>
>> [...]
>>
>>>> -int raw_pci_write(unsigned int domain, unsigned int bus,
>>>> -        unsigned int devfn, int reg, int len, u32 val)
>>>> +struct pci_ops pci_root_ops = {
>>>> +    .map_bus = pci_mcfg_dev_base,
>>>> +    .read = pci_generic_config_read,
>>>> +    .write = pci_generic_config_write,
>>>
>>>
>>> Can you change these with pci_generic_config_read32 and
>>> pci_generic_config_write32? We have some targets that can only do 32
>>> bits PCI config space access.
>>
>> No.
>>
>> http://www.spinics.net/lists/linux-pci/msg44869.html
>>
>> Can you be a bit more specific please ?
>>
>> Sigh. Looks like we have to start adding platform specific quirks even
>> before we merged the generic ACPI PCIe host controller implementation.
>
> Cc Gab, Zhou, and Dondong who upstream the hip05 (designware) PCIe host
> support.
>
> I think so, some platform may not support ECAM for root complex,
> which needs special handling of access config space, we may need
> to consider those cases.
>

Yes, it is indeed true.  For example, some Cavium ThunderX processors 
fall into this category.

Some options I thought of are:

  o Use DECLARE_ACPI_MCFG_FIXUP() in the kernel to supply the needed 
config space accessors.

  o Define additional root_device_ids that imply the needed config space 
accessors.


> Thanks
> Hanjun

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

Sinan Kaya Nov. 3, 2015, 5:43 p.m. UTC | #21

On 11/3/2015 11:55 AM, Arnd Bergmann wrote:
> On Tuesday 03 November 2015 11:33:18 Sinan Kaya wrote:
>>
>> On 11/3/2015 10:59 AM, Arnd Bergmann wrote:
>>> On Tuesday 03 November 2015 10:10:21 Sinan Kaya wrote:
>>>>
>>>> I don't see anywhere in the SBSA spec addendum that the PCI
>>>> configuration space section that unaligned accesses *MUST* be supported.
>>>>
>>>> If this is required, please have this info added to the spec. I can work
>>>> with the designers for the next chip.
>>>>
>>>> Unaligned access on the current hardware returns incomplete values or
>>>> can cause bus faults. The behavior is undefined.
>>>
>>> Unaligned accesses are not allowed, but any PCI compliant device must
>>> support aligned 1, 2 or 4 byte accesses on its configuration space,
>>> though the byte-enable mechanism. In an ECAM host bridge, those are
>>> mapped to load/store accesses from the CPU with the respective width
>>> and natural alignment.
>>
>> As far as I see, the endpoints do not have any problems with unaligned
>> accesses. It is the host bridge itself (stuff that doesn't get on the
>> PCIe bus and uses traditional AXI kind bus internally) has problems with
>> alignment.
>>
>> If Linux is expecting all HW vendors to implement alignment support,
>> this needs to be put in the SBSA spec as a hard requirement.
>
> As I said, it's not unaligned accesses at all, just 1-byte and aligned
> 2-byte accesses, and it's not Linux mandating this but the PCI
> spec. Please read Russell's email again, it is not possible for PCI
> to work according to the specification unless the host bridge allows
> sub-32-bit accesses.

I'll check back with the hardware designers. Seeing readb/readw/readl 
made me nervous that we are trying unaligned access from any boundaries.

In any case, the hardware document says 32 bit configuration space 
access to the host bridge only. I'll get more clarification.

>
> You can probably work around this by using the legacy I/O port method
> rather than ECAM, if the PCI host bridge itself is functional and just
> the host bus it is connected to is buggy.

 From the sounds of it, we'll need a quirk for config space. We support 
legacy I/O only to make the endpoints happy. Some endpoints do not get 
initialized if they don't have a BAR address assigned to all the BAR 
resources.

I just saw David Daney's email. I like his idea. I think this chip will 
fit into the same category.

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

Gabriele Paoloni Nov. 3, 2015, 6 p.m. UTC | #22

Hi David

> -----Original Message-----
> From: David Daney [mailto:[e-mail]]
> Sent: 03 November 2015 17:39
> To: Hanjun Guo
> Cc: Lorenzo Pieralisi; Sinan Kaya; Tomasz Nowicki; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Wangyijing; [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]; Gabriele Paoloni; Wangzhou
> (B); liudongdong (C)
> Subject: Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI
> hostbridge init
> 
> On 11/03/2015 07:19 AM, Hanjun Guo wrote:
> > On 11/03/2015 10:15 PM, Lorenzo Pieralisi wrote:
> >> On Wed, Oct 28, 2015 at 02:46:37PM -0400, Sinan Kaya wrote:
> >>
> >> [...]
> >>
> >>>> -int raw_pci_write(unsigned int domain, unsigned int bus,
> >>>> -        unsigned int devfn, int reg, int len, u32 val)
> >>>> +struct pci_ops pci_root_ops = {
> >>>> +    .map_bus = pci_mcfg_dev_base,
> >>>> +    .read = pci_generic_config_read,
> >>>> +    .write = pci_generic_config_write,
> >>>
> >>>
> >>> Can you change these with pci_generic_config_read32 and
> >>> pci_generic_config_write32? We have some targets that can only do 32
> >>> bits PCI config space access.
> >>
> >> No.
> >>
> >> http://www.spinics.net/lists/linux-pci/msg44869.html
> >>
> >> Can you be a bit more specific please ?
> >>
> >> Sigh. Looks like we have to start adding platform specific quirks even
> >> before we merged the generic ACPI PCIe host controller implementation.
> >
> > Cc Gab, Zhou, and Dondong who upstream the hip05 (designware) PCIe host
> > support.
> >
> > I think so, some platform may not support ECAM for root complex,
> > which needs special handling of access config space, we may need
> > to consider those cases.
> >
> 
> Yes, it is indeed true.  For example, some Cavium ThunderX processors
> fall into this category.
> 
> Some options I thought of are:
> 
>   o Use DECLARE_ACPI_MCFG_FIXUP() in the kernel to supply the needed
> config space accessors.
> 
>   o Define additional root_device_ids that imply the needed config space
> accessors.

Yes I like this

it would fit designware too

Gab

> 
> 
> > Thanks
> > Hanjun

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

Tomasz Nowicki Nov. 4, 2015, 9:59 a.m. UTC | #23

On 03.11.2015 17:55, Lorenzo Pieralisi wrote:
> On Tue, Oct 27, 2015 at 05:38:42PM +0100, Tomasz Nowicki wrote:
>
> [...]
>
>>   menu "Kernel Features"
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index b3d098b..66cc1ae 100644
>> --- a/arch/arm64/kernel/pci.c
>> +++ b/arch/arm64/kernel/pci.c
>> @@ -11,12 +11,15 @@
>>    */
>>
>>   #include <linux/acpi.h>
>> +#include <linux/ecam.h>
>>   #include <linux/init.h>
>>   #include <linux/io.h>
>>   #include <linux/kernel.h>
>>   #include <linux/mm.h>
>> +#include <linux/of_address.h>
>>   #include <linux/of_pci.h>
>>   #include <linux/of_platform.h>
>> +#include <linux/pci-acpi.h>
>>   #include <linux/slab.h>
>>
>>   #include <asm/pci-bridge.h>
>> @@ -52,35 +55,216 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
>>   }
>>
>>   /*
>> - * Try to assign the IRQ number from DT when adding a new device
>> + * Try to assign the IRQ number from DT/ACPI when adding a new device
>>    */
>>   int pcibios_add_device(struct pci_dev *dev)
>>   {
>> -	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
>> +	if (acpi_disabled)
>> +		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
>> +#ifdef CONFIG_ACPI
>> +	else
>> +		acpi_pci_irq_enable(dev);
>> +#endif
>
> This series:
>
> http://www.spinics.net/lists/linux-pci/msg45950.html
>
> will allow us to initialize the irq mapping function according to
> the boot method, code above is getting cumbersome and it is already
> overriden when booting with DT, so we will remove it altogether.
>
>>
>>   	return 0;
>>   }
>>
>> +#ifdef CONFIG_ACPI
>> +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
>> +{
>> +	struct acpi_pci_root *root = bridge->bus->sysdata;
>> +
>> +	ACPI_COMPANION_SET(&bridge->dev, root->device);
>> +	return 0;
>
> This should be made part of core code IMO.
>
>> +}
>> +
>> +void pcibios_add_bus(struct pci_bus *bus)
>> +{
>> +	acpi_pci_add_bus(bus);
>> +}
>> +
>> +void pcibios_remove_bus(struct pci_bus *bus)
>> +{
>> +	acpi_pci_remove_bus(bus);
>> +}
>
> Two functions above are identical for arm64, ia64 and x86, I do
> not think they belong in arch code.
>
>> +static int __init pcibios_assign_resources(void)
>> +{
>> +	if (acpi_disabled)
>> +		return 0;
>> +
>> +	pci_assign_unassigned_resources();
>> +	return 0;
>
> Already commented on this.
>
>> +}
>>   /*
>> - * raw_pci_read/write - Platform-specific PCI config space access.
>> + * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
>> + * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
>>    */
>> -int raw_pci_read(unsigned int domain, unsigned int bus,
>> -		  unsigned int devfn, int reg, int len, u32 *val)
>> +rootfs_initcall(pcibios_assign_resources);
>> +
>> +static void __iomem *
>> +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
>>   {
>> -	return -ENXIO;
>> +	struct pci_mmcfg_region *cfg;
>> +
>> +	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
>> +	if (cfg && cfg->virt)
>> +		return cfg->virt +
>> +			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
>> +			offset;
>> +	return NULL;
>
> Why is this code arm64 specific ?
>
>>   }
>>
>> -int raw_pci_write(unsigned int domain, unsigned int bus,
>> -		unsigned int devfn, int reg, int len, u32 val)
>> +struct pci_ops pci_root_ops = {
>> +	.map_bus = pci_mcfg_dev_base,
>> +	.read = pci_generic_config_read,
>> +	.write = pci_generic_config_write,
>> +};
>> +
>> +#ifdef CONFIG_PCI_MMCONFIG
>> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
>>   {
>> -	return -ENXIO;
>> +	struct pci_mmcfg_region *cfg;
>> +	struct acpi_pci_root *root;
>> +	int seg, start, end, err;
>> +
>> +	root = ci->root;
>> +	seg = root->segment;
>> +	start = root->secondary.start;
>> +	end = root->secondary.end;
>> +
>> +	cfg = pci_mmconfig_lookup(seg, start);
>> +	if (cfg)
>> +		return 0;
>> +
>> +	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
>> +	if (!cfg)
>> +		return -ENOMEM;
>> +
>> +	err = pci_mmconfig_inject(cfg);
>> +	return err;
>>   }
>>
>> -#ifdef CONFIG_ACPI
>> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
>> +{
>> +	struct acpi_pci_root *root = ci->root;
>> +	struct pci_mmcfg_region *cfg;
>> +
>> +	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
>> +	if (cfg)
>> +		return;
>> +
>> +	if (cfg->hot_added)
>> +		pci_mmconfig_delete(root->segment, root->secondary.start,
>> +				    root->secondary.end);
>> +}
>> +#else
>> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
>> +{
>> +	return 0;
>> +}
>> +
>> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
>> +#endif
>
> Ditto.
>
>> +
>> +static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
>> +{
>> +	return pci_add_mmconfig_region(ci);
>> +}
>> +
>> +static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
>> +{
>> +	pci_remove_mmconfig_region(ci);
>> +	kfree(ci);
>> +}
>> +
>> +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
>> +{
>> +	struct resource_entry *entry, *tmp;
>> +	int ret;
>> +
>> +	ret = acpi_pci_probe_root_resources(ci);
>> +	if (ret < 0)
>> +		return ret;
>
> Code above is identical on arm64, ia64 and x86.
>
>> +
>> +	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
>> +		struct resource *res = entry->res;
>> +
>> +		/*
>> +		 * Special handling for ARM IO range
>> +		 * 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;
>> +
>> +			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 - 1;
>> +
>> +			if (pci_remap_iospace(res, res->start) < 0)
>> +				resource_list_destroy_entry(entry);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct acpi_pci_root_ops acpi_pci_root_ops = {
>> +	.pci_ops = &pci_root_ops,
>> +	.init_info = pci_acpi_root_init_info,
>> +	.release_info = pci_acpi_root_release_info,
>> +	.prepare_resources = pci_acpi_root_prepare_resources,
>> +};
>> +
>>   /* Root bridge scanning */
>>   struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>>   {
>> -	/* TODO: Should be revisited when implementing PCI on ACPI */
>> -	return NULL;
>> +	int node = acpi_get_node(root->device->handle);
>> +	int domain = root->segment;
>> +	int busnum = root->secondary.start;
>> +	struct acpi_pci_root_info *info;
>> +	struct pci_bus *bus;
>> +
>> +	if (domain && !pci_domains_supported) {
>> +		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
>> +			domain, busnum);
>> +		return NULL;
>> +	}
>> +
>> +	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
>> +	if (!info) {
>> +		dev_err(&root->device->dev,
>> +			"pci_bus %04x:%02x: ignored (out of memory)\n",
>> +			domain, busnum);
>> +		return NULL;
>> +	}
>> +
>> +	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
>> +
>> +	/* After the PCI-E bus has been walked and all devices discovered,
>> +	 * configure any settings of the fabric that might be necessary.
>> +	 */
>> +	if (bus) {
>> +		struct pci_bus *child;
>> +
>> +		list_for_each_entry(child, &bus->children, node)
>> +			pcie_bus_configure_settings(child);
>> +	}
>> +
>> +	return bus;
>
> Code above is entirely arch agnostic (apart from what data is passed to
> sysdata) and I think there is room for further consolidation with
> x86 and ia64, I will have a look into this.
>

I agree with your comments, currently I do not know how much of it can 
be consolidated but I will rework my next version in this direction.

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

Tomasz Nowicki Nov. 4, 2015, 10:11 a.m. UTC | #24

On 03.11.2015 17:55, Lorenzo Pieralisi wrote:
> On Tue, Oct 27, 2015 at 05:38:42PM +0100, Tomasz Nowicki wrote:
>
> [...]
>
>>   menu "Kernel Features"
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index b3d098b..66cc1ae 100644
>> --- a/arch/arm64/kernel/pci.c
>> +++ b/arch/arm64/kernel/pci.c
>> @@ -11,12 +11,15 @@
>>    */
>>
>>   #include <linux/acpi.h>
>> +#include <linux/ecam.h>
>>   #include <linux/init.h>
>>   #include <linux/io.h>
>>   #include <linux/kernel.h>
>>   #include <linux/mm.h>
>> +#include <linux/of_address.h>
>>   #include <linux/of_pci.h>
>>   #include <linux/of_platform.h>
>> +#include <linux/pci-acpi.h>
>>   #include <linux/slab.h>
>>
>>   #include <asm/pci-bridge.h>
>> @@ -52,35 +55,216 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
>>   }
>>
>>   /*
>> - * Try to assign the IRQ number from DT when adding a new device
>> + * Try to assign the IRQ number from DT/ACPI when adding a new device
>>    */
>>   int pcibios_add_device(struct pci_dev *dev)
>>   {
>> -	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
>> +	if (acpi_disabled)
>> +		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
>> +#ifdef CONFIG_ACPI
>> +	else
>> +		acpi_pci_irq_enable(dev);
>> +#endif
>
> This series:
>
> http://www.spinics.net/lists/linux-pci/msg45950.html
>
> will allow us to initialize the irq mapping function according to
> the boot method, code above is getting cumbersome and it is already
> overriden when booting with DT, so we will remove it altogether.
>
>>
>>   	return 0;
>>   }
>>
>> +#ifdef CONFIG_ACPI
>> +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
>> +{
>> +	struct acpi_pci_root *root = bridge->bus->sysdata;
>> +
>> +	ACPI_COMPANION_SET(&bridge->dev, root->device);
>> +	return 0;
>
> This should be made part of core code IMO.
>
>> +}
>> +
>> +void pcibios_add_bus(struct pci_bus *bus)
>> +{
>> +	acpi_pci_add_bus(bus);
>> +}
>> +
>> +void pcibios_remove_bus(struct pci_bus *bus)
>> +{
>> +	acpi_pci_remove_bus(bus);
>> +}
>
> Two functions above are identical for arm64, ia64 and x86, I do
> not think they belong in arch code.
>
>> +static int __init pcibios_assign_resources(void)
>> +{
>> +	if (acpi_disabled)
>> +		return 0;
>> +
>> +	pci_assign_unassigned_resources();
>> +	return 0;
>
> Already commented on this.
>
>> +}
>>   /*
>> - * raw_pci_read/write - Platform-specific PCI config space access.
>> + * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
>> + * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
>>    */
>> -int raw_pci_read(unsigned int domain, unsigned int bus,
>> -		  unsigned int devfn, int reg, int len, u32 *val)
>> +rootfs_initcall(pcibios_assign_resources);
>> +
>> +static void __iomem *
>> +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
>>   {
>> -	return -ENXIO;
>> +	struct pci_mmcfg_region *cfg;
>> +
>> +	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
>> +	if (cfg && cfg->virt)
>> +		return cfg->virt +
>> +			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
>> +			offset;
>> +	return NULL;
>
> Why is this code arm64 specific ?

It is not, I will move it out of here, probably to mcfg.c file where we 
can apply quirks.

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

Sinan Kaya Nov. 5, 2015, 2:48 p.m. UTC | #25

On 11/3/2015 12:43 PM, Sinan Kaya wrote:
> In any case, the hardware document says 32 bit configuration space
> access to the host bridge only. I'll get more clarification.
>
I got confirmation this morning that this chip supports 32 bit access to 
the root complex configuration space. 8/16/32 bits accesses to the 
endpoints are supported.

>>
>> You can probably work around this by using the legacy I/O port method
>> rather than ECAM, if the PCI host bridge itself is functional and just
>> the host bus it is connected to is buggy.
>
>  From the sounds of it, we'll need a quirk for config space. We support
> legacy I/O only to make the endpoints happy. Some endpoints do not get
> initialized if they don't have a BAR address assigned to all the BAR
> resources.

We'll need an MCFG fix up.

Patch

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 07d1811..bbcc6b1 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -89,6 +89,7 @@  config ARM64
 	select SPARSE_IRQ
 	select SYSCTL_EXCEPTION_TRACE
 	select HAVE_CONTEXT_TRACKING
+	select HAVE_PCI_ECAM
 	help
 	  ARM 64-bit (AArch64) Linux support.
 
@@ -202,6 +203,11 @@  source "drivers/pci/Kconfig"
 source "drivers/pci/pcie/Kconfig"
 source "drivers/pci/hotplug/Kconfig"
 
+config PCI_MMCONFIG
+	def_bool y
+	select PCI_ECAM
+	depends on ACPI
+
 endmenu
 
 menu "Kernel Features"
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index b3d098b..66cc1ae 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -11,12 +11,15 @@ 
  */
 
 #include <linux/acpi.h>
+#include <linux/ecam.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
+#include <linux/of_address.h>
 #include <linux/of_pci.h>
 #include <linux/of_platform.h>
+#include <linux/pci-acpi.h>
 #include <linux/slab.h>
 
 #include <asm/pci-bridge.h>
@@ -52,35 +55,216 @@  int pcibios_enable_device(struct pci_dev *dev, int mask)
 }
 
 /*
- * Try to assign the IRQ number from DT when adding a new device
+ * Try to assign the IRQ number from DT/ACPI when adding a new device
  */
 int pcibios_add_device(struct pci_dev *dev)
 {
-	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+	if (acpi_disabled)
+		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+#ifdef CONFIG_ACPI
+	else
+		acpi_pci_irq_enable(dev);
+#endif
 
 	return 0;
 }
 
+#ifdef CONFIG_ACPI
+int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
+{
+	struct acpi_pci_root *root = bridge->bus->sysdata;
+
+	ACPI_COMPANION_SET(&bridge->dev, root->device);
+	return 0;
+}
+
+void pcibios_add_bus(struct pci_bus *bus)
+{
+	acpi_pci_add_bus(bus);
+}
+
+void pcibios_remove_bus(struct pci_bus *bus)
+{
+	acpi_pci_remove_bus(bus);
+}
+
+static int __init pcibios_assign_resources(void)
+{
+	if (acpi_disabled)
+		return 0;
+
+	pci_assign_unassigned_resources();
+	return 0;
+}
 /*
- * raw_pci_read/write - Platform-specific PCI config space access.
+ * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
+ * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
  */
-int raw_pci_read(unsigned int domain, unsigned int bus,
-		  unsigned int devfn, int reg, int len, u32 *val)
+rootfs_initcall(pcibios_assign_resources);
+
+static void __iomem *
+pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
 {
-	return -ENXIO;
+	struct pci_mmcfg_region *cfg;
+
+	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
+	if (cfg && cfg->virt)
+		return cfg->virt +
+			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
+			offset;
+	return NULL;
 }
 
-int raw_pci_write(unsigned int domain, unsigned int bus,
-		unsigned int devfn, int reg, int len, u32 val)
+struct pci_ops pci_root_ops = {
+	.map_bus = pci_mcfg_dev_base,
+	.read = pci_generic_config_read,
+	.write = pci_generic_config_write,
+};
+
+#ifdef CONFIG_PCI_MMCONFIG
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
 {
-	return -ENXIO;
+	struct pci_mmcfg_region *cfg;
+	struct acpi_pci_root *root;
+	int seg, start, end, err;
+
+	root = ci->root;
+	seg = root->segment;
+	start = root->secondary.start;
+	end = root->secondary.end;
+
+	cfg = pci_mmconfig_lookup(seg, start);
+	if (cfg)
+		return 0;
+
+	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
+	if (!cfg)
+		return -ENOMEM;
+
+	err = pci_mmconfig_inject(cfg);
+	return err;
 }
 
-#ifdef CONFIG_ACPI
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+	struct acpi_pci_root *root = ci->root;
+	struct pci_mmcfg_region *cfg;
+
+	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
+	if (cfg)
+		return;
+
+	if (cfg->hot_added)
+		pci_mmconfig_delete(root->segment, root->secondary.start,
+				    root->secondary.end);
+}
+#else
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+	return 0;
+}
+
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
+#endif
+
+static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
+{
+	return pci_add_mmconfig_region(ci);
+}
+
+static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
+{
+	pci_remove_mmconfig_region(ci);
+	kfree(ci);
+}
+
+static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
+{
+	struct resource_entry *entry, *tmp;
+	int ret;
+
+	ret = acpi_pci_probe_root_resources(ci);
+	if (ret < 0)
+		return ret;
+
+	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
+		struct resource *res = entry->res;
+
+		/*
+		 * Special handling for ARM IO range
+		 * 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;
+
+			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 - 1;
+
+			if (pci_remap_iospace(res, res->start) < 0)
+				resource_list_destroy_entry(entry);
+		}
+	}
+
+	return 0;
+}
+
+static struct acpi_pci_root_ops acpi_pci_root_ops = {
+	.pci_ops = &pci_root_ops,
+	.init_info = pci_acpi_root_init_info,
+	.release_info = pci_acpi_root_release_info,
+	.prepare_resources = pci_acpi_root_prepare_resources,
+};
+
 /* Root bridge scanning */
 struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 {
-	/* TODO: Should be revisited when implementing PCI on ACPI */
-	return NULL;
+	int node = acpi_get_node(root->device->handle);
+	int domain = root->segment;
+	int busnum = root->secondary.start;
+	struct acpi_pci_root_info *info;
+	struct pci_bus *bus;
+
+	if (domain && !pci_domains_supported) {
+		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
+			domain, busnum);
+		return NULL;
+	}
+
+	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
+	if (!info) {
+		dev_err(&root->device->dev,
+			"pci_bus %04x:%02x: ignored (out of memory)\n",
+			domain, busnum);
+		return NULL;
+	}
+
+	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
+
+	/* After the PCI-E bus has been walked and all devices discovered,
+	 * configure any settings of the fabric that might be necessary.
+	 */
+	if (bus) {
+		struct pci_bus *child;
+
+		list_for_each_entry(child, &bus->children, node)
+			pcie_bus_configure_settings(child);
+	}
+
+	return bus;
 }
 #endif