From mboxrd@z Thu Jan 1 00:00:00 1970 From: Liviu Dudau Subject: Re: [RFC PATCH 4/4] arm64, acpi, pci: Provide arch-specific calls for PCI host bridge dirver (PNP0A03). Date: Fri, 7 Nov 2014 14:55:25 +0000 Message-ID: <20141107145525.GH8916@e106497-lin.cambridge.arm.com> References: <1415366876-30811-1-git-send-email-tomasz.nowicki@linaro.org> <1415366876-30811-5-git-send-email-tomasz.nowicki@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from service87.mimecast.com ([91.220.42.44]:35477 "EHLO service87.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753133AbaKGOza convert rfc822-to-8bit (ORCPT ); Fri, 7 Nov 2014 09:55:30 -0500 In-Reply-To: <1415366876-30811-5-git-send-email-tomasz.nowicki@linaro.org> Content-Disposition: inline Sender: linux-acpi-owner@vger.kernel.org List-Id: linux-acpi@vger.kernel.org To: Tomasz Nowicki Cc: Catalin Marinas , Will Deacon , "bhelgaas@google.com" , "tglx@linutronix.de" , "mingo@redhat.com" , "hpa@zytor.com" , "rjw@rjwysocki.net" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "x86@kernel.org" , "linux-pci@vger.kernel.org" , "linux-acpi@vger.kernel.org" , "linaro-acpi@lists.linaro.org" On Fri, Nov 07, 2014 at 01:27:56PM +0000, Tomasz Nowicki wrote: > Code inspired by arch/x86/pci/acpi.c and arch/ia64/pci/pci.c files. > The main reasons why we need this: > - parse and manage resources (BUS, IO and MEM) > - create pci root bus and enumerate its children > - provide PCI config space accessors (via MMCONFIG) Hi Tomasz, I have some comments to your code: >=20 > Signed-off-by: Tomasz Nowicki > --- > arch/arm64/include/asm/pci.h | 8 + > arch/arm64/kernel/pci.c | 401 +++++++++++++++++++++++++++++++++= ++++++++-- > 2 files changed, 391 insertions(+), 18 deletions(-) >=20 > diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pc= i.h > index fded096..ecd940f 100644 > --- a/arch/arm64/include/asm/pci.h > +++ b/arch/arm64/include/asm/pci.h > @@ -33,6 +33,14 @@ static inline int pci_get_legacy_ide_irq(struct pc= i_dev *dev, int channel) > extern int isa_dma_bridge_buggy; >=20 > #ifdef CONFIG_PCI > +struct pci_controller { > + struct acpi_device *companion; > + int segment; > + int node; /* nearest node with memory or NUMA_N= O_NODE for global allocation */ > +}; > + > +#define PCI_CONTROLLER(busdev) ((struct pci_controller *) busdev->sy= sdata) I am trying to move away from the model where the architecture dictates= the shape of the pci_controller structure. Can I suggest that you put all this co= de and the one under #ifdef CONFIG_ACPI in a separate file, say arch/arm64/kernel/= pci-acpi.c ? Or that you add an #ifdef CONFIG_ACPI around this structure definition? I can't see anyone using this definition outside ACPI (due to struct ac= pi_device dependency) and I would like to avoid it conflicting with other host br= idge drivers trying to define the same name structure. > + > static inline int pci_proc_domain(struct pci_bus *bus) > { > return 1; > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c > index 42fb195..cc34ded 100644 > --- a/arch/arm64/kernel/pci.c > +++ b/arch/arm64/kernel/pci.c > @@ -1,6 +1,10 @@ > /* > - * Code borrowed from powerpc/kernel/pci-common.c > + * Code borrowed from powerpc/kernel/pci-common.c and arch/ia64/pci/= pci.c > * > + * Copyright (c) 2002, 2005 Hewlett-Packard Development Company, L.P= =2E > + * David Mosberger-Tang > + * Bjorn Helgaas > + * Copyright (C) 2004 Silicon Graphics, Inc. > * Copyright (C) 2003 Anton Blanchard , IBM > * Copyright (C) 2014 ARM Ltd. > * > @@ -17,10 +21,16 @@ > #include > #include > #include > +#include > #include > +#include > +#include > +#include >=20 > #include >=20 > +#define PREFIX "PCI: " Where is this used? > + > /* > * Called after each bus is probed, but before its children are exam= ined > */ > @@ -43,7 +53,8 @@ resource_size_t pcibios_align_resource(void *data, = const struct resource *res, > */ > int pcibios_add_device(struct pci_dev *dev) > { > - dev->irq =3D of_irq_parse_and_map_pci(dev, 0, 0); > + if (acpi_disabled) > + dev->irq =3D of_irq_parse_and_map_pci(dev, 0, 0); >=20 > return 0; > } > @@ -54,45 +65,399 @@ static bool dt_domain_found =3D false; >=20 > void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *pa= rent) > { > - int domain =3D of_get_pci_domain_nr(parent->of_node); > + int domain =3D -1; >=20 > - if (domain >=3D 0) { > - dt_domain_found =3D true; > - } else if (dt_domain_found =3D=3D true) { > - dev_err(parent, "Node %s is missing \"linux,pci-domai= n\" property in DT\n", > - parent->of_node->full_name); > - return; > + if (acpi_disabled) { > + domain =3D of_get_pci_domain_nr(parent->of_node); > + > + if (domain >=3D 0) { > + dt_domain_found =3D true; > + } else if (dt_domain_found =3D=3D true) { > + dev_err(parent, "Node %s is missing \"linux,p= ci-domain\" property in DT\n", > + parent->of_node->full_name); > + return; > + } else { > + domain =3D pci_get_new_domain_nr(); > + } > } else { > - domain =3D pci_get_new_domain_nr(); > + /* > + * Take the domain nr from associated private data. > + * Case where bus has parent is covered in pci_alloc_= bus() > + */ > + if (!parent) > + domain =3D PCI_CONTROLLER(bus)->segment; I would also like to wrap this into an ACPI specific function. Reason f= or it is that when I get to split the pci_host_bridge creation out of pci_cre= ate_root_bus() this will become a pci_controller property. > } >=20 > bus->domain_nr =3D domain; > } > #endif >=20 > +static char __iomem *pci_dev_base(unsigned int seg, unsigned int bus= , > + unsigned int devfn) > +{ > + struct pci_mmcfg_region *cfg =3D pci_mmconfig_lookup(seg, bus= ); > + > + if (cfg && cfg->virt) > + return cfg->virt + (PCI_MMCFG_BUS_OFFSET(bus) | (devf= n << 12)); > + return NULL; > +} > + > /* > * raw_pci_read/write - Platform-specific PCI config space access. > - * > - * Default empty implementation. Replace with an architecture-speci= fic setup > - * routine, if necessary. > */ > int raw_pci_read(unsigned int domain, unsigned int bus, > unsigned int devfn, int reg, int len, u32 *val) > { > - return -EINVAL; > + char __iomem *addr; > + > + if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) { > +err: *val =3D -1; I believe the general usage is to have labels on their own line. > + return -EINVAL; > + } > + > + rcu_read_lock(); > + addr =3D pci_dev_base(domain, bus, devfn); > + if (!addr) { > + rcu_read_unlock(); > + goto err; > + } > + > + switch (len) { > + case 1: > + *val =3D readb(addr + reg); > + break; > + case 2: > + *val =3D readw(addr + reg); > + break; > + case 4: > + *val =3D readl(addr + reg); > + break; > + } > + rcu_read_unlock(); > + > + return 0; > } >=20 > int raw_pci_write(unsigned int domain, unsigned int bus, > unsigned int devfn, int reg, int len, u32 val) > { > - return -EINVAL; > + char __iomem *addr; > + > + if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) > + return -EINVAL; > + > + rcu_read_lock(); > + addr =3D pci_dev_base(domain, bus, devfn); > + if (!addr) { > + rcu_read_unlock(); > + return -EINVAL; > + } > + > + switch (len) { > + case 1: > + writeb(val, addr + reg); > + break; > + case 2: > + writew(val, addr + reg); > + break; > + case 4: > + writel(val, addr + reg); > + break; > + } > + rcu_read_unlock(); > + > + return 0; > } >=20 These raw_pci_{read,write} functions are all ACPI specific so they can = move into the new file as well. > #ifdef CONFIG_ACPI > +static int pci_read(struct pci_bus *bus, unsigned int devfn, int whe= re, > + int size, u32 *value) > +{ > + return raw_pci_read(pci_domain_nr(bus), bus->number, > + devfn, where, size, value); > +} > + > +static int pci_write(struct pci_bus *bus, unsigned int devfn, int wh= ere, > + int size, u32 value) > +{ > + return raw_pci_write(pci_domain_nr(bus), bus->number, > + devfn, where, size, value); > +} > + > +struct pci_ops pci_root_ops =3D { > + .read =3D pci_read, > + .write =3D pci_write, > +}; > + > +static struct pci_controller *alloc_pci_controller(int seg) > +{ > + struct pci_controller *controller; > + > + controller =3D kzalloc(sizeof(*controller), GFP_KERNEL); > + if (!controller) > + return NULL; > + > + controller->segment =3D seg; > + return controller; > +} > + > +struct pci_root_info { > + struct acpi_device *bridge; > + struct pci_controller *controller; > + struct list_head resources; > + struct resource *res; > + resource_size_t *res_offset; Why do you need to keep an array of res_offsets here? You only seem to be using the values once when you add the resource to the host bridge windows. > + unsigned int res_num; > + char *name; > +}; > + > +static acpi_status resource_to_window(struct acpi_resource *resource= , > + struct acpi_resource_address64 = *addr) > +{ > + acpi_status status; > + > + /* > + * We're only interested in _CRS descriptors that are > + * - address space descriptors for memory > + * - non-zero size > + * - producers, i.e., the address space is routed downst= ream, > + * not consumed by the bridge itself > + */ > + status =3D acpi_resource_to_address64(resource, addr); > + if (ACPI_SUCCESS(status) && > + (addr->resource_type =3D=3D ACPI_MEMORY_RANGE || > + addr->resource_type =3D=3D ACPI_IO_RANGE) && > + addr->address_length && > + addr->producer_consumer =3D=3D ACPI_PRODUCER) > + return AE_OK; > + > + return AE_ERROR; > +} > + > +static acpi_status count_window(struct acpi_resource *resource, void= *data) > +{ > + unsigned int *windows =3D (unsigned int *) data; > + struct acpi_resource_address64 addr; > + acpi_status status; > + > + status =3D resource_to_window(resource, &addr); > + if (ACPI_SUCCESS(status)) > + (*windows)++; > + > + return AE_OK; > +} > + > +static acpi_status add_window(struct acpi_resource *res, void *data) > +{ > + struct pci_root_info *info =3D data; > + struct resource *resource; > + struct acpi_resource_address64 addr; > + acpi_status status; > + unsigned long flags; > + struct resource *root; > + u64 start; > + > + /* Return AE_OK for non-window resources to keep scanning for= more */ > + status =3D resource_to_window(res, &addr); > + if (!ACPI_SUCCESS(status)) > + return AE_OK; > + > + if (addr.resource_type =3D=3D ACPI_MEMORY_RANGE) { > + flags =3D IORESOURCE_MEM; > + root =3D &iomem_resource; > + } else if (addr.resource_type =3D=3D ACPI_IO_RANGE) { > + flags =3D IORESOURCE_IO; > + root =3D &ioport_resource; > + } else > + return AE_OK; > + > + start =3D addr.minimum + addr.translation_offset; > + > + resource =3D &info->res[info->res_num]; > + resource->name =3D info->name; > + resource->flags =3D flags; > + resource->start =3D start; > + resource->end =3D resource->start + addr.address_length - 1; > + > + if (flags & IORESOURCE_IO) { > + unsigned long port; > + int err; > + > + err =3D pci_register_io_range(start, addr.address_len= gth); > + if (err) > + return AE_OK; > + > + port =3D pci_address_to_pio(start); > + if (port =3D=3D (unsigned long)-1) > + return AE_OK; > + > + resource->start =3D port; > + resource->end =3D port + addr.address_length - 1; > + > + if (pci_remap_iospace(resource, start) < 0) > + return AE_OK; You seem to leave around invalid resources every time you return in thi= s code path due to your choice of ignoring error conditions. I think there are a few things that can be streamlined in this patch, b= ut overall I think it looks promising. Best regards, Liviu > + > + info->res_offset[info->res_num] =3D 0; > + } else > + info->res_offset[info->res_num] =3D addr.translation_= offset; > + > + if (insert_resource(root, resource)) { > + dev_err(&info->bridge->dev, > + "can't allocate host bridge window %pR\n", > + resource); > + } else { > + if (addr.translation_offset) > + dev_info(&info->bridge->dev, "host bridge win= dow %pR " > + "(PCI address [%#llx-%#llx])\n", > + resource, > + resource->start - addr.translation_o= ffset, > + resource->end - addr.translation_off= set); > + else > + dev_info(&info->bridge->dev, > + "host bridge window %pR\n", resource= ); > + } > + > + pci_add_resource_offset(&info->resources, resource, > + info->res_offset[info->res_num]); > + info->res_num++; > + return AE_OK; > +} > + > +static void free_pci_root_info_res(struct pci_root_info *info) > +{ > + kfree(info->name); > + kfree(info->res); > + info->res =3D NULL; > + kfree(info->res_offset); > + info->res_offset =3D NULL; > + info->res_num =3D 0; > + kfree(info->controller); > + info->controller =3D NULL; > +} > + > +static void __release_pci_root_info(struct pci_root_info *info) > +{ > + int i; > + struct resource *res; > + > + for (i =3D 0; i < info->res_num; i++) { > + res =3D &info->res[i]; > + > + if (!res->parent) > + continue; > + > + if (!(res->flags & (IORESOURCE_MEM | IORESOURCE_IO))) > + continue; > + > + release_resource(res); > + } > + > + free_pci_root_info_res(info); > + kfree(info); > +} > + > +static void release_pci_root_info(struct pci_host_bridge *bridge) > +{ > + struct pci_root_info *info =3D bridge->release_data; > + > + __release_pci_root_info(info); > +} > + > +static int > +probe_pci_root_info(struct pci_root_info *info, struct acpi_device *= device, > + int busnum, int domain) > +{ > + char *name; > + > + name =3D kmalloc(16, GFP_KERNEL); > + if (!name) > + return -ENOMEM; > + > + sprintf(name, "PCI Bus %04x:%02x", domain, busnum); > + info->bridge =3D device; > + info->name =3D name; > + > + acpi_walk_resources(device->handle, METHOD_NAME__CRS, count_w= indow, > + &info->res_num); > + if (info->res_num) { > + info->res =3D > + kzalloc_node(sizeof(*info->res) * info->res_n= um, > + GFP_KERNEL, info->controller->no= de); > + if (!info->res) { > + kfree(name); > + return -ENOMEM; > + } > + > + info->res_offset =3D > + kzalloc_node(sizeof(*info->res_offset) * info= ->res_num, > + GFP_KERNEL, info->controller-= >node); > + if (!info->res_offset) { > + kfree(name); > + kfree(info->res); > + info->res =3D NULL; > + return -ENOMEM; > + } > + > + info->res_num =3D 0; > + acpi_walk_resources(device->handle, METHOD_NAME__CRS, > + add_window, info); > + } else > + kfree(name); > + > + return 0; > +} > + > /* 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; > + struct acpi_device *device =3D root->device; > + int domain =3D root->segment; > + int bus =3D root->secondary.start; > + struct pci_controller *controller; > + struct pci_root_info *info =3D NULL; > + int busnum =3D root->secondary.start; > + struct pci_bus *pbus; > + int ret; > + > + controller =3D alloc_pci_controller(domain); > + if (!controller) > + return NULL; > + > + controller->companion =3D device; > + controller->node =3D acpi_get_node(device->handle); > + > + info =3D kzalloc(sizeof(*info), GFP_KERNEL); > + if (!info) { > + dev_err(&device->dev, > + "pci_bus %04x:%02x: ignored (out of m= emory)\n", > + domain, busnum); > + kfree(controller); > + return NULL; > + } > + > + info->controller =3D controller; > + INIT_LIST_HEAD(&info->resources); > + > + ret =3D probe_pci_root_info(info, device, busnum, domain); > + if (ret) { > + kfree(info->controller); > + kfree(info); > + return NULL; > + } > + /* insert busn resource at first */ > + pci_add_resource(&info->resources, &root->secondary); > + > + pbus =3D pci_create_root_bus(NULL, bus, &pci_root_ops, contro= ller, > + &info->resources); > + if (!pbus) { > + pci_free_resource_list(&info->resources); > + __release_pci_root_info(info); > + return NULL; > + } > + > + pci_set_host_bridge_release(to_pci_host_bridge(pbus->bridge), > + release_pci_root_info, info); > + pci_scan_child_bus(pbus); > + return pbus; > } > -#endif > +#endif /* CONFIG_ACPI */ > -- > 1.9.1 >=20 >=20 --=20 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- =C2=AF\_(=E3=83=84)_/=C2=AF -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" i= n the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html