Messages in this thread | | | Subject | Re: [PATCH V6 07/13] PCI: Provide common functions for ECAM mapping | From | Tomasz Nowicki <> | Date | Thu, 5 May 2016 12:38:54 +0200 |
| |
On 05.05.2016 11:24, Jayachandran C wrote: > On Fri, Apr 29, 2016 at 1:31 PM, Jayachandran C <jchandra@broadcom.com> wrote: >> On Fri, Apr 29, 2016 at 3:17 AM, Bjorn Helgaas <helgaas@kernel.org> wrote: >>> On Fri, Apr 15, 2016 at 07:06:42PM +0200, Tomasz Nowicki wrote: >>>> From: Jayachandran C <jchandra@broadcom.com> >>>> >>>> Add config option PCI_GENERIC_ECAM and file drivers/pci/ecam.c to >>>> provide generic functions for accessing memory mapped PCI config space. >>>> >>>> The API is defined in drivers/pci/ecam.h and is written to replace the >>>> API in drivers/pci/host/pci-host-common.h. The file defines a new >>>> 'struct pci_config_window' to hold the information related to a PCI >>>> config area and its mapping. This structure is expected to be used as >>>> sysdata for controllers that have ECAM based mapping. >>>> >>>> Helper functions are provided to setup the mapping, free the mapping >>>> and to implement the map_bus method in 'struct pci_ops' >>> >>> Spec reference: PCI Express Base Specification, rev 3.0, sec 7.2.2. >>> >>>> Signed-off-by: Jayachandran C <jchandra@broadcom.com> >>>> --- >>>> drivers/pci/Kconfig | 3 ++ >>>> drivers/pci/Makefile | 2 + >>>> drivers/pci/ecam.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> drivers/pci/ecam.h | 61 +++++++++++++++++++++++ >>>> 4 files changed, 203 insertions(+) >>>> create mode 100644 drivers/pci/ecam.c >>>> create mode 100644 drivers/pci/ecam.h >>>> >>>> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig >>>> index 209292e..e930d62 100644 >>>> --- a/drivers/pci/Kconfig >>>> +++ b/drivers/pci/Kconfig >>>> @@ -83,6 +83,9 @@ config HT_IRQ >>>> config PCI_ATS >>>> bool >>>> >>>> +config PCI_GENERIC_ECAM >>>> + bool >>> >>> "PCI_ECAM" is enough, I think. It's defined by and required by the >>> spec unless there's some arch-specific interface. Plus, if I >> >> Ok. Looking at the comments I think I have to take out generic from >> all the names - will do this in next version. >> >>> understand correctly, this infrastructure supports non-generic ECAM >>> implementations as well, since the caller supplies "struct >>> pci_generic_ecam_ops *ops". >> >> Yes, the idea was to support ECAM with quirks (and CAM) on both >> 32 and 64 bit, otherwise it would be too trivial to have a separate >> implementation. >> >>>> config PCI_IOV >>>> bool "PCI IOV support" >>>> depends on PCI >>>> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile >>>> index 2154092..810aec8 100644 >>>> --- a/drivers/pci/Makefile >>>> +++ b/drivers/pci/Makefile >>>> @@ -55,6 +55,8 @@ obj-$(CONFIG_PCI_SYSCALL) += syscall.o >>>> >>>> obj-$(CONFIG_PCI_STUB) += pci-stub.o >>>> >>>> +obj-$(CONFIG_PCI_GENERIC_ECAM) += ecam.o >>>> + >>>> obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o >>>> >>>> obj-$(CONFIG_OF) += of.o >>>> diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c >>>> new file mode 100644 >>>> index 0000000..ff04c01 >>>> --- /dev/null >>>> +++ b/drivers/pci/ecam.c >>>> @@ -0,0 +1,137 @@ >>>> +/* >>>> + * Copyright 2016 Broadcom >>>> + * >>>> + * This program is free software; you can redistribute it and/or modify >>>> + * it under the terms of the GNU General Public License, version 2, as >>>> + * published by the Free Software Foundation (the "GPL"). >>>> + * >>>> + * This program is distributed in the hope that it will be useful, but >>>> + * WITHOUT ANY WARRANTY; without even the implied warranty of >>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >>>> + * General Public License version 2 (GPLv2) for more details. >>>> + * >>>> + * You should have received a copy of the GNU General Public License >>>> + * version 2 (GPLv2) along with this source code. >>>> + */ >>>> + >>>> +#include <linux/device.h> >>>> +#include <linux/io.h> >>>> +#include <linux/kernel.h> >>>> +#include <linux/module.h> >>>> +#include <linux/pci.h> >>>> +#include <linux/slab.h> >>>> + >>>> +#include "ecam.h" >>>> + >>>> +/* >>>> + * On 64 bit systems, we do a single ioremap for the whole config space >>>> + * since we have enough virtual address range available. On 32 bit, do an >>>> + * ioremap per bus. >>>> + */ >>>> +static const bool per_bus_mapping = !config_enabled(CONFIG_64BIT); >>>> + >>>> +/* >>>> + * Create a PCI config space window >>>> + * - reserve mem region >>>> + * - alloc struct pci_config_window with space for all mappings >>>> + * - ioremap the config space >>>> + */ >>>> +struct pci_config_window *pci_generic_ecam_create(struct device *dev, >>>> + phys_addr_t addr, u8 bus_start, u8 bus_end, >>> >>> Can you take pointers to struct resources here instead of addr, >>> bus_start, and bus_end? The caller probably has them already, and >>> then you could add a useful printk like: >>> >>> dev_info(dev, "ECAM for %pR at %pR\n", busn_res, mmio_res); >>> >>> Would have to be careful about the struct resource lifetimes though. >> >> Yes, I had thought of this. The reason I did not go down that path was >> that we are using request_mem_region() which takes the address >> and creates a resource .internally. >> >> Beyond that, as you noted, the ownership and lifetime is slightly >> more complex. Either the calling code has to allocate the >> resource and handoff, or the ecam code has to make a copy of >> the resource. I would go with copy since it is much more simple >> to use. >> >> Since resource based API seems to be preferred, I will switch >> to passing bus and mmio resource and will use >> request_resource_conflict() to allocate the ECAM mem region, >> >>> If you had the MMIO resource here, you could also do the range >>> checking you currently have in gen_pci_init() here instead, so all >>> callers could benefit. >> >> Ok. >> >>>> + struct pci_generic_ecam_ops *ops) >>>> +{ >>>> + struct pci_config_window *cfg; >>>> + unsigned int bus_shift, bus_range, bsz, mapsz; >>>> + int i, nidx; >>>> + int err = -ENOMEM; >>>> + >>>> + if (bus_end < bus_start) >>>> + return ERR_PTR(-EINVAL); >>>> + >>>> + bus_shift = ops->bus_shift; >>>> + bus_range = bus_end - bus_start + 1; >>>> + bsz = 1 << bus_shift; >>>> + nidx = per_bus_mapping ? bus_range : 1; >>>> + mapsz = per_bus_mapping ? bsz : bus_range * bsz; >>>> + cfg = kzalloc(sizeof(*cfg) + nidx * sizeof(cfg->win[0]), GFP_KERNEL); >>>> + if (!cfg) >>>> + return ERR_PTR(-ENOMEM); >>>> + >>>> + cfg->bus_start = bus_start; >>>> + cfg->bus_end = bus_end; >>>> + cfg->ops = ops; >>>> + >>>> + if (!request_mem_region(addr, bus_range * bsz, "Configuration Space")) >>>> + goto err_exit; >>>> + >>>> + /* cfgaddr has to be set after request_mem_region */ >>>> + cfg->cfgaddr = addr; >>>> + >>>> + for (i = 0; i < nidx; i++) { >>>> + cfg->win[i] = ioremap(addr + i * mapsz, mapsz); >>>> + if (!cfg->win[i]) >>>> + goto err_exit; >>>> + } >>>> + >>>> + if (cfg->ops->init) { >>>> + err = cfg->ops->init(dev, cfg); >>>> + if (err) >>>> + goto err_exit; >>>> + } >>>> + return cfg; >>>> + >>>> +err_exit: >>>> + pci_generic_ecam_free(cfg); >>>> + return ERR_PTR(err); >>>> +} >>>> + >>>> +/* >>>> + * Free a config space mapping >>>> + */ >>> >>> Superfluous comment. >> >> Ok, will trim comments to keep the ones that are useful. >> >>>> +void pci_generic_ecam_free(struct pci_config_window *cfg) >>>> +{ >>>> + unsigned int bus_range; >>>> + int i, nidx; >>>> + >>>> + bus_range = cfg->bus_end - cfg->bus_start + 1; >>>> + nidx = per_bus_mapping ? bus_range : 1; >>>> + for (i = 0; i < nidx; i++) >>>> + if (cfg->win[i]) >>>> + iounmap(cfg->win[i]); >>>> + if (cfg->cfgaddr) >>>> + release_mem_region(cfg->cfgaddr, >>>> + bus_range << cfg->ops->bus_shift); >>>> + kfree(cfg); >>>> +} >>>> + >>>> +/* >>>> + * Function to implement the pci_ops ->map_bus method >>>> + */ >>>> +void __iomem *pci_generic_ecam_map_bus(struct pci_bus *bus, unsigned int devfn, >>>> + int where) >>>> +{ >>>> + struct pci_config_window *cfg = bus->sysdata; >>> >>> I don't really like the use of bus->sysdata here, because sysdata is >>> explicitly arch-specific. >>> >>> But I guess we're in a bind right now: it'd be nice to save the cfg >>> pointer in struct pci_host_bridge, but you have to call >>> pci_generic_ecam_create() before the struct pci_host_bridge has been >>> allocated, and you have to pass the pointer into pci_scan_root_bus(), >>> and there's no generic way to do that yet. >>> >>> So I guess this will have to do for now. >> >> I could call the structure pci_ecam_sysdata instead of pci_config_window >> to make it clear that it is to be used as sysdata for ECAM based PCI host >> controllers. >> >> The current push (at least on ARM/ARM64) seems to be to make >> bus->sysdata controller specific and avoid arch specific sysdata. >> >>>> + unsigned int devfn_shift = cfg->ops->bus_shift - 8; >>>> + unsigned int busn = bus->number; >>>> + void __iomem *base; >>>> + >>>> + if (busn < cfg->bus_start || busn > cfg->bus_end) >>>> + return NULL; >>>> + >>>> + busn -= cfg->bus_start; >>>> + if (per_bus_mapping) >>>> + base = cfg->win[busn]; >>>> + else >>>> + base = cfg->win[0] + (busn << cfg->ops->bus_shift); >>>> + return base + (devfn << devfn_shift) + where; >>>> +} >>>> + >>>> +/* default ECAM ops */ >>>> +struct pci_generic_ecam_ops pci_generic_ecam_default_ops = { >>> >>> Using both "generic" and "default" seems overkill. Maybe just: >>> >>> struct pci_ecam_ops pci_generic_ecam_ops = { ... ? >> >> Ok. >> >>>> + .bus_shift = 20, >>>> + .pci_ops = { >>>> + .map_bus = pci_generic_ecam_map_bus, >>>> + .read = pci_generic_config_read, >>>> + .write = pci_generic_config_write, >>>> + } >>>> +}; >>>> diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h >>>> new file mode 100644 >>>> index 0000000..34c0aba >>>> --- /dev/null >>>> +++ b/drivers/pci/ecam.h >>>> @@ -0,0 +1,61 @@ >>>> +/* >>>> + * Copyright 2016 Broadcom >>>> + * >>>> + * This program is free software; you can redistribute it and/or modify >>>> + * it under the terms of the GNU General Public License, version 2, as >>>> + * published by the Free Software Foundation (the "GPL"). >>>> + * >>>> + * This program is distributed in the hope that it will be useful, but >>>> + * WITHOUT ANY WARRANTY; without even the implied warranty of >>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >>>> + * General Public License version 2 (GPLv2) for more details. >>>> + * >>>> + * You should have received a copy of the GNU General Public License >>>> + * version 2 (GPLv2) along with this source code. >>>> + */ >>>> +#ifndef DRIVERS_PCI_ECAM_H >>>> +#define DRIVERS_PCI_ECAM_H >>>> + >>>> +#include <linux/kernel.h> >>>> +#include <linux/platform_device.h> >>>> + >>>> +/* >>>> + * struct to hold pci ops and bus shift of the config window >>>> + * for a PCI controller. >>>> + */ >>>> +struct pci_config_window; >>>> +struct pci_generic_ecam_ops { >>> >>> "struct pci_ecam_ops" >> >> Ok. >> >>>> + unsigned int bus_shift; >>>> + struct pci_ops pci_ops; >>>> + int (*init)(struct device *, >>>> + struct pci_config_window *); >>>> +}; >>>> + >>>> +/* >>>> + * struct to hold the mappings of a config space window. This >>>> + * will be allocated with enough entries in win[] to hold all >>>> + * the mappings for the bus range. >>>> + */ >>>> +struct pci_config_window { >>>> + phys_addr_t cfgaddr; >>>> + u16 domain; >>>> + u8 bus_start; >>>> + u8 bus_end; >>>> + void *priv; >>>> + struct pci_generic_ecam_ops *ops; >>>> + void __iomem *win[0]; >>>> +}; >>>> + >>>> +/* create and free for pci_config_window */ >>> >>> Superfluous comment. >>> >>>> +struct pci_config_window *pci_generic_ecam_create(struct device *dev, >>>> + phys_addr_t addr, u8 bus_start, u8 bus_end, >>>> + struct pci_generic_ecam_ops *ops); >>>> +void pci_generic_ecam_free(struct pci_config_window *cfg); >>> >>> "pci_ecam_create" and "pci_ecam_free"? I suspect you're going to call >>> these for flavors of ECAM that are definitely not "generic". >>> >>>> +/* map_bus when ->sysdata is an instance of pci_config_window */ >>>> +void __iomem *pci_generic_ecam_map_bus(struct pci_bus *bus, unsigned int devfn, >>>> + int where); >>>> +/* default ECAM ops, bus shift 20, generic read and write */ >>>> +extern struct pci_generic_ecam_ops pci_generic_ecam_default_ops; >>>> + >>>> +#endif >> >> Thanks for the review. The ECAM patchset can be merged separately >> if you do not want to tie it the ACPI changes. >> >> Please let me know how you want to proceed. Depending on that, I will >> either post it as a separate patchset or ask Tomasz to pick up my >> changes and post the ACPI patchset. again. > > I have made the changes suggested here, and added one more > change to move from 'void __iomem *win[0]' to a union for > pci_config_window so that it can be embedded in other structures[1]. > > The changes are at https://github.com/jchandra-brcm/linux branch > arm64-acpi-pci-v4 so that Tomasz can add it to v7 of the ACPI/PCI > patches. I am not posting the changes here to avoid the impression > that there are clashing ACPI/PCI patchsets. > > The branch also has an example patch for ACPI generic PCI root > using the new ECAM code. This also has the simpler domain > setting code for ACPI as well as fix for the ECAM address > calculation which came up during the earlier discussion. > > JC. > [1] Minor API changes will be needed for embedding now. > This change will make it easier to merge with Arnd's host bridge > changes when that goes in. >
Thanks JC!
Tomasz
| |