Message-ID: <20160119115815.GC5276@red-moon>
Date:	Tue, 19 Jan 2016 11:58:15 +0000
From:	Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To:	Tomasz Nowicki <tn@...ihalf.com>
Cc:	bhelgaas@...gle.com, arnd@...db.de, will.deacon@....com,
	catalin.marinas@....com, rjw@...ysocki.net, hanjun.guo@...aro.org,
	okaya@...eaurora.org, jiang.liu@...ux.intel.com,
	Stefano.Stabellini@...citrix.com,
	robert.richter@...iumnetworks.com, mw@...ihalf.com,
	Liviu.Dudau@....com, ddaney@...iumnetworks.com, tglx@...utronix.de,
	wangyijing@...wei.com, Suravee.Suthikulpanit@....com,
	msalter@...hat.com, linux-pci@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-acpi@...r.kernel.org,
	linux-kernel@...r.kernel.org, linaro-acpi@...ts.linaro.org,
	jchandra@...adcom.com, jcm@...hat.com
Subject: Re: [PATCH V3 19/21] pci, acpi: Support for ACPI based generic PCI
 host controller init

On Wed, Jan 13, 2016 at 02:21:05PM +0100, Tomasz Nowicki wrote:
> Because of two patch series:
> 1. Jiang Liu's common interface to support PCI host controller init
> 2. MMCONFIG refactoring (part of this patch set)
> now we can think about generic ACPI based PCI host controller init
> implementation out of arch/ directory.
> 
> These calls use information from MCFG table (PCI config space regions)
> and _CRS method (IO/irq resources) to initialize PCI hostbridge.
> 
> TBD: We are still not sure whether we should reassign resources
> after PCI bus enumeration or trust firmware to do all that work for
> us properly.

We should claim resources and assign unassigned ones. I put together a
patch for resource claiming instead of reinventing the wheel, waiting
for feedback:

https://patchwork.ozlabs.org/patch/545669/

If we merge the code with no resources claiming, we may end up in
situations where claiming can trigger regressions so we won't be able
to do it anymore.

> Signed-off-by: Tomasz Nowicki <tn@...ihalf.com>
> Signed-off-by: Hanjun Guo <hanjun.guo@...aro.org>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
> CC: Arnd Bergmann <arnd@...db.de>
> CC: Catalin Marinas <catalin.marinas@....com>
> CC: Liviu Dudau <Liviu.Dudau@....com>
> CC: Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>
> CC: Will Deacon <will.deacon@....com>
> Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
> Tested-by: Jeremy Linton <jeremy.linton@....com>
> ---
>  drivers/acpi/Kconfig    |   5 ++
>  drivers/acpi/pci_root.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 136 insertions(+)
> 
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index c3664be..e315061 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -335,6 +335,11 @@ config ACPI_PCI_SLOT
>  	  i.e., segment/bus/device/function tuples, with physical slots in
>  	  the system.  If you are unsure, say N.
>  
> +config ACPI_PCI_HOST_GENERIC
> +	bool "Generic ACPI PCI host controller"
> +	help
> +	  Say Y here if you want to support generic ACPI PCI host controller.

You should add a proper description here.

> +
>  config X86_PM_TIMER
>  	bool "Power Management Timer Support" if EXPERT
>  	depends on X86
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index a65c8c2..d483e2a 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -24,6 +24,7 @@
>  #include <linux/init.h>
>  #include <linux/types.h>
>  #include <linux/mutex.h>
> +#include <linux/of_address.h>

We should move the IO space management to PCI core instead of having
it in OF core, code carrying out PIO mapping does not depend on OF
as far as I can see.

>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pci.h>
> @@ -514,6 +515,136 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
>  	}
>  }
>  
> +#ifdef CONFIG_ACPI_PCI_HOST_GENERIC
> +static int pcibios_map_irq(struct pci_dev *dev, u8 slot, u8 pin)
> +{
> +	if (pci_dev_msi_enabled(dev))
> +		return 0;
> +
> +	acpi_pci_irq_enable(dev);
> +	return dev->irq;
> +}
> +
> +static void pci_mcfg_release_info(struct acpi_pci_root_info *ci)
> +{
> +	pci_mmcfg_teardown_map(ci);
> +	kfree(ci);
> +}
> +
> +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> +{
> +	struct list_head *list = &ci->resources;
> +	struct acpi_device *device = ci->bridge;
> +	struct resource_entry *entry, *tmp;
> +	unsigned long flags;
> +	int ret;
> +
> +	flags = IORESOURCE_IO | IORESOURCE_MEM;
> +	ret = acpi_dev_get_resources(device, list,
> +				     acpi_dev_filter_resource_type_cb,
> +				     (void *)flags);
> +	if (ret < 0) {
> +		dev_warn(&device->dev,
> +			 "failed to parse _CRS method, error code %d\n", ret);
> +		return ret;
> +	} else if (ret == 0)
> +		dev_dbg(&device->dev,
> +			"no IO and memory resources present in _CRS\n");
		^^^^
		what's the point in carrying on then ?

> +
> +	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
> +		resource_size_t cpu_addr, length;
> +		struct resource *res = entry->res;
> +
> +		if (entry->res->flags & IORESOURCE_DISABLED)
> +			resource_list_destroy_entry(entry);
> +		else
> +			res->name = ci->name;
> +
> +		/* PCI -> CPU space translation */
> +		cpu_addr = res->start + entry->offset;
> +		length = res->end - res->start + 1;
> +
> +		if (res->flags & IORESOURCE_MEM) {
> +			res->start = cpu_addr;
> +			res->end = cpu_addr + length - 1;
> +		} else if (res->flags & IORESOURCE_IO) {
> +			resource_size_t pci_addr = res->start;
> +			unsigned long port;
> +
> +			if (pci_register_io_range(cpu_addr, length)) {
> +				resource_list_destroy_entry(entry);
> +				continue;
> +			}
> +
> +			port = pci_address_to_pio(cpu_addr);
> +			if (port == (unsigned long)-1) {
> +				resource_list_destroy_entry(entry);
> +				continue;
> +			}
> +
> +			res->start = port;
> +			res->end = port + length - 1;
> +			entry->offset = port - pci_addr;
> +
> +			if (pci_remap_iospace(res, cpu_addr) < 0)
> +				resource_list_destroy_entry(entry);
> +		}
> +	}
> +	return ret;
> +}
> +
> +static struct acpi_pci_root_ops acpi_pci_root_ops = {
> +	.init_info = pci_mmcfg_setup_map,
> +	.release_info = pci_mcfg_release_info,
> +	.prepare_resources = pci_acpi_root_prepare_resources,
> +};
> +
> +/* Root bridge scanning */
> +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> +{
> +	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_host_bridge *bridge;
> +	struct pci_bus *bus, *child;
> +
> +	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;
> +	}
> +
> +	acpi_pci_root_ops.pci_ops = pci_mcfg_get_ops(root);
> +	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
> +	if (!bus)
> +		return NULL;
		^^^
		Leaking memory here.

> +
> +	bridge = pci_find_host_bridge(bus);
> +	bridge->map_irq = pcibios_map_irq;

It would be nice to use map_irq for that, but Matthew's series seems
stuck in review mode, either we take that series on and make some
progress on it or you should add the irq mapping code to arm64 arch
code, *temporarily* :)

Also, we should claim resources here.

> +
> +	pci_bus_size_bridges(bus);
> +	pci_bus_assign_resources(bus);
> +
> +	/*
> +	 * After the PCI-E bus has been walked and all devices discovered,
> +	 * configure any settings of the fabric that might be necessary.
> +	 */
> +	list_for_each_entry(child, &bus->children, node)
> +		pcie_bus_configure_settings(child);
> +
> +	return bus;
> +}
> +#endif /* CONFIG_ARCH_PCI_HOST_GENERIC_ACPI */
             ^^^
	     does not match the #ifdef

Lorenzo

Powered by blists - more mailing lists