* [RESEND PATCH 0/3] Support for Generic PCI Host Controller
@ 2014-05-02 16:41 Will Deacon
2014-05-02 16:41 ` [RESEND PATCH 1/3] ARM: mach-virt: allow PCI support to be selected Will Deacon
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Will Deacon @ 2014-05-02 16:41 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: arnd, linux-pci, bhelgaas, jgunthorpe, Will Deacon
Hi all,
This is a resend of the generic PCI host controller patch I last posted
in March:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-March/237540.html
I've also included two additional patches to enable PCI for mach-virt
and add a MAINTAINERS entry for the new driver (Arnd -- I'm happy to add
you here as well if you like).
I've tested successfully on a 3.15-rc3-based kernel with kvmtool and would
really like to see this merged for 3.16.
All feedback welcome,
Will
Will Deacon (3):
ARM: mach-virt: allow PCI support to be selected
PCI: ARM: add support for generic PCI host controller
MAINTAINERS: add entry for generic PCI host controller driver
.../devicetree/bindings/pci/host-generic-pci.txt | 100 ++++++
MAINTAINERS | 8 +
arch/arm/Kconfig | 1 +
drivers/pci/host/Kconfig | 7 +
drivers/pci/host/Makefile | 1 +
drivers/pci/host/pci-host-generic.c | 369 +++++++++++++++++++++
6 files changed, 486 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/host-generic-pci.txt
create mode 100644 drivers/pci/host/pci-host-generic.c
--
1.9.2
^ permalink raw reply [flat|nested] 15+ messages in thread* [RESEND PATCH 1/3] ARM: mach-virt: allow PCI support to be selected 2014-05-02 16:41 [RESEND PATCH 0/3] Support for Generic PCI Host Controller Will Deacon @ 2014-05-02 16:41 ` Will Deacon 2014-05-02 18:11 ` Arnd Bergmann 2014-05-02 16:41 ` [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller Will Deacon 2014-05-02 16:41 ` [RESEND PATCH 3/3] MAINTAINERS: add entry for generic PCI host controller driver Will Deacon 2 siblings, 1 reply; 15+ messages in thread From: Will Deacon @ 2014-05-02 16:41 UTC (permalink / raw) To: linux-arm-kernel; +Cc: arnd, linux-pci, bhelgaas, jgunthorpe, Will Deacon mach-virt can make use of virtio-pci devices, which requires the guest kernel to have PCI support selected. This patch selects CONFIG_MIGHT_HAVE_PCI when CONFIG_ARCH_VIRT=y. Signed-off-by: Will Deacon <will.deacon@arm.com> --- arch/arm/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index db3c5414223e..f24356c4d630 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -941,6 +941,7 @@ config ARCH_VIRT select ARM_GIC select ARM_PSCI select HAVE_ARM_ARCH_TIMER + select MIGHT_HAVE_PCI # # This is sorted alphabetically by mach-* pathname. However, plat-* -- 1.9.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 1/3] ARM: mach-virt: allow PCI support to be selected 2014-05-02 16:41 ` [RESEND PATCH 1/3] ARM: mach-virt: allow PCI support to be selected Will Deacon @ 2014-05-02 18:11 ` Arnd Bergmann 2014-05-02 18:21 ` Will Deacon 0 siblings, 1 reply; 15+ messages in thread From: Arnd Bergmann @ 2014-05-02 18:11 UTC (permalink / raw) To: Will Deacon; +Cc: linux-arm-kernel, linux-pci, bhelgaas, jgunthorpe On Friday 02 May 2014 17:41:14 Will Deacon wrote: > mach-virt can make use of virtio-pci devices, which requires the guest > kernel to have PCI support selected. > > This patch selects CONFIG_MIGHT_HAVE_PCI when CONFIG_ARCH_VIRT=y. > > Signed-off-by: Will Deacon <will.deacon@arm.com> > --- > arch/arm/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index db3c5414223e..f24356c4d630 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -941,6 +941,7 @@ config ARCH_VIRT > select ARM_GIC > select ARM_PSCI > select HAVE_ARM_ARCH_TIMER > + select MIGHT_HAVE_PCI I would actually just put it under ARCH_MULTIPLATFORM. Anything can have PCI really, and it doesn't cause any harm. Arnd ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 1/3] ARM: mach-virt: allow PCI support to be selected 2014-05-02 18:11 ` Arnd Bergmann @ 2014-05-02 18:21 ` Will Deacon 0 siblings, 0 replies; 15+ messages in thread From: Will Deacon @ 2014-05-02 18:21 UTC (permalink / raw) To: Arnd Bergmann; +Cc: linux-arm-kernel, linux-pci, bhelgaas, jgunthorpe Hi Arnd, On Fri, May 02, 2014 at 07:11:04PM +0100, Arnd Bergmann wrote: > On Friday 02 May 2014 17:41:14 Will Deacon wrote: > > mach-virt can make use of virtio-pci devices, which requires the guest > > kernel to have PCI support selected. > > > > This patch selects CONFIG_MIGHT_HAVE_PCI when CONFIG_ARCH_VIRT=y. > > > > Signed-off-by: Will Deacon <will.deacon@arm.com> > > --- > > arch/arm/Kconfig | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > > index db3c5414223e..f24356c4d630 100644 > > --- a/arch/arm/Kconfig > > +++ b/arch/arm/Kconfig > > @@ -941,6 +941,7 @@ config ARCH_VIRT > > select ARM_GIC > > select ARM_PSCI > > select HAVE_ARM_ARCH_TIMER > > + select MIGHT_HAVE_PCI > > I would actually just put it under ARCH_MULTIPLATFORM. Anything can > have PCI really, and it doesn't cause any harm. Sure, I can change that easily enough. Will ^ permalink raw reply [flat|nested] 15+ messages in thread
* [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller 2014-05-02 16:41 [RESEND PATCH 0/3] Support for Generic PCI Host Controller Will Deacon 2014-05-02 16:41 ` [RESEND PATCH 1/3] ARM: mach-virt: allow PCI support to be selected Will Deacon @ 2014-05-02 16:41 ` Will Deacon 2014-05-02 17:23 ` Jason Gunthorpe 2014-05-02 16:41 ` [RESEND PATCH 3/3] MAINTAINERS: add entry for generic PCI host controller driver Will Deacon 2 siblings, 1 reply; 15+ messages in thread From: Will Deacon @ 2014-05-02 16:41 UTC (permalink / raw) To: linux-arm-kernel; +Cc: arnd, linux-pci, bhelgaas, jgunthorpe, Will Deacon This patch adds support for a generic PCI host controller, such as a firmware-initialised device with static windows or an emulation by something such as kvmtool. The controller itself has no configuration registers and has its address spaces described entirely by the device-tree (using the bindings from ePAPR). Both CAM and ECAM are supported for Config Space accesses. Corresponding documentation is added for the DT binding. Reviewed-by: Liviu Dudau <liviu.dudau@arm.com> Signed-off-by: Will Deacon <will.deacon@arm.com> --- .../devicetree/bindings/pci/host-generic-pci.txt | 100 ++++++ drivers/pci/host/Kconfig | 7 + drivers/pci/host/Makefile | 1 + drivers/pci/host/pci-host-generic.c | 369 +++++++++++++++++++++ 4 files changed, 477 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/host-generic-pci.txt create mode 100644 drivers/pci/host/pci-host-generic.c diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.txt b/Documentation/devicetree/bindings/pci/host-generic-pci.txt new file mode 100644 index 000000000000..f0b0436807b4 --- /dev/null +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt @@ -0,0 +1,100 @@ +* Generic PCI host controller + +Firmware-initialised PCI host controllers and PCI emulations, such as the +virtio-pci implementations found in kvmtool and other para-virtualised +systems, do not require driver support for complexities such as regulator +and clock management. In fact, the controller may not even require the +configuration of a control interface by the operating system, instead +presenting a set of fixed windows describing a subset of IO, Memory and +Configuration Spaces. + +Such a controller can be described purely in terms of the standardized device +tree bindings communicated in pci.txt: + + +Properties of the host controller node: + +- compatible : Must be "pci-host-cam-generic" or "pci-host-ecam-generic" + depending on the layout of configuration space (CAM vs + ECAM respectively). + +- device_type : Must be "pci". + +- ranges : As described in IEEE Std 1275-1994, but must provide + at least a definition of non-prefetchable memory. One + or both of prefetchable Memory and IO Space may also + be provided. + +- bus-range : Optional property (also described in IEEE Std 1275-1994) + to indicate the range of bus numbers for this controller. + If absent, defaults to <0 255> (i.e. all buses). + +- #address-cells : Must be 3. + +- #size-cells : Must be 2. + +- reg : The Configuration Space base address and size, as accessed + from the parent bus. + + +Properties of the /chosen node: + +- linux,pci-probe-only + : Optional property which takes a single-cell argument. + If '0', then Linux will assign devices in its usual manner, + otherwise it will not try to assign devices and instead use + them as they are configured already. + +Configuration Space is assumed to be memory-mapped (as opposed to being +accessed via an ioport) and laid out with a direct correspondence to the +geography of a PCI bus address by concatenating the various components to +form an offset. + +For CAM, this 24-bit offset is: + + cfg_offset(bus, device, function, register) = + bus << 16 | device << 11 | function << 8 | register + +Whilst ECAM extends this by 4 bits to accomodate 4k of function space: + + cfg_offset(bus, device, function, register) = + bus << 20 | device << 15 | function << 12 | register + +Interrupt mapping is exactly as described in `Open Firmware Recommended +Practice: Interrupt Mapping' and requires the following properties: + +- #interrupt-cells : Must be 1 + +- interrupt-map : <see aforementioned specification> + +- interrupt-map-mask : <see aforementioned specification> + + +Example: + +pci { + compatible = "pci-host-cam-generic" + device_type = "pci"; + #address-cells = <3>; + #size-cells = <2>; + bus-range = <0x0 0x1>; + + // CPU_PHYSICAL(2) SIZE(2) + reg = <0x0 0x40000000 0x0 0x1000000>; + + // BUS_ADDRESS(3) CPU_PHYSICAL(2) SIZE(2) + ranges = <0x01000000 0x0 0x01000000 0x0 0x01000000 0x0 0x00010000>, + <0x02000000 0x0 0x41000000 0x0 0x41000000 0x0 0x3f000000>; + + + #interrupt-cells = <0x1>; + + // PCI_DEVICE(3) INT#(1) CONTROLLER(PHANDLE) CONTROLLER_DATA(3) + interrupt-map = < 0x0 0x0 0x0 0x1 &gic 0x0 0x4 0x1 + 0x800 0x0 0x0 0x1 &gic 0x0 0x5 0x1 + 0x1000 0x0 0x0 0x1 &gic 0x0 0x6 0x1 + 0x1800 0x0 0x0 0x1 &gic 0x0 0x7 0x1>; + + // PCI_DEVICE(3) INT#(1) + interrupt-map-mask = <0xf800 0x0 0x0 0x7>; +} diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig index a6f67ec8882f..32d446effbb3 100644 --- a/drivers/pci/host/Kconfig +++ b/drivers/pci/host/Kconfig @@ -33,4 +33,11 @@ config PCI_RCAR_GEN2 There are 3 internal PCI controllers available with a single built-in EHCI/OHCI host controller present on each one. +config PCI_HOST_GENERIC + bool "Generic PCI host controller" + depends on ARM && OF + help + Say Y here if you want to support a simple generic PCI host + controller, such as the one emulated by kvmtool. + endmenu diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile index 13fb3333aa05..bd1bf1ab4ac8 100644 --- a/drivers/pci/host/Makefile +++ b/drivers/pci/host/Makefile @@ -4,3 +4,4 @@ obj-$(CONFIG_PCI_IMX6) += pci-imx6.o obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o obj-$(CONFIG_PCI_TEGRA) += pci-tegra.o obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o +obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c new file mode 100644 index 000000000000..b761f3e64a6a --- /dev/null +++ b/drivers/pci/host/pci-host-generic.c @@ -0,0 +1,369 @@ +/* + * Simple, generic PCI host controller driver targetting firmware-initialised + * systems and virtual machines (e.g. the PCI emulation provided by kvmtool). + * + * 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. + * + * 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 for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + * + * Copyright (C) 2014 ARM Limited + * + * Author: Will Deacon <will.deacon@arm.com> + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/of_pci.h> +#include <linux/platform_device.h> + +struct gen_pci_cfg_bus_ops { + u32 bus_shift; + void __iomem *(*map_bus)(struct pci_bus *, unsigned int, int); +}; + +struct gen_pci_cfg_windows { + struct resource res; + struct resource bus_range; + void __iomem **win; + + const struct gen_pci_cfg_bus_ops *ops; +}; + +struct gen_pci { + struct pci_host_bridge host; + struct gen_pci_cfg_windows cfg; +}; + +static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus, + unsigned int devfn, + int where) +{ + struct pci_sys_data *sys = bus->sysdata; + struct gen_pci *pci = sys->private_data; + resource_size_t idx = bus->number - pci->cfg.bus_range.start; + + return pci->cfg.win[idx] + ((devfn << 8) | where); +} + +static struct gen_pci_cfg_bus_ops gen_pci_cfg_cam_bus_ops = { + .bus_shift = 16, + .map_bus = gen_pci_map_cfg_bus_cam, +}; + +static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus, + unsigned int devfn, + int where) +{ + struct pci_sys_data *sys = bus->sysdata; + struct gen_pci *pci = sys->private_data; + resource_size_t idx = bus->number - pci->cfg.bus_range.start; + + return pci->cfg.win[idx] + ((devfn << 12) | where); +} + +static struct gen_pci_cfg_bus_ops gen_pci_cfg_ecam_bus_ops = { + .bus_shift = 20, + .map_bus = gen_pci_map_cfg_bus_ecam, +}; + +static int gen_pci_config_read(struct pci_bus *bus, unsigned int devfn, + int where, int size, u32 *val) +{ + void __iomem *addr; + struct pci_sys_data *sys = bus->sysdata; + struct gen_pci *pci = sys->private_data; + + addr = pci->cfg.ops->map_bus(bus, devfn, where); + + switch (size) { + case 1: + *val = readb(addr); + break; + case 2: + *val = readw(addr); + break; + default: + *val = readl(addr); + } + + return PCIBIOS_SUCCESSFUL; +} + +static int gen_pci_config_write(struct pci_bus *bus, unsigned int devfn, + int where, int size, u32 val) +{ + void __iomem *addr; + struct pci_sys_data *sys = bus->sysdata; + struct gen_pci *pci = sys->private_data; + + addr = pci->cfg.ops->map_bus(bus, devfn, where); + + switch (size) { + case 1: + writeb(val, addr); + break; + case 2: + writew(val, addr); + break; + default: + writel(val, addr); + } + + return PCIBIOS_SUCCESSFUL; +} + +static struct pci_ops gen_pci_ops = { + .read = gen_pci_config_read, + .write = gen_pci_config_write, +}; + +static int gen_pci_calc_io_offset(struct device *dev, + struct of_pci_range *range, + struct resource *res, + resource_size_t *offset) +{ + static atomic_t wins = ATOMIC_INIT(0); + int err, idx, max_win; + unsigned int window; + + if (!PAGE_ALIGNED(range->cpu_addr)) + return -EINVAL; + + max_win = (IO_SPACE_LIMIT + 1) / SZ_64K; + idx = atomic_inc_return(&wins); + if (idx >= max_win) + return -ENOSPC; + + window = (idx - 1) * SZ_64K; + err = pci_ioremap_io(window, range->cpu_addr); + if (err) + return err; + + of_pci_range_to_resource(range, dev->of_node, res); + res->start = window; + res->end = res->start + range->size - 1; + *offset = window - range->pci_addr; + return 0; +} + +static int gen_pci_calc_mem_offset(struct device *dev, + struct of_pci_range *range, + struct resource *res, + resource_size_t *offset) +{ + of_pci_range_to_resource(range, dev->of_node, res); + *offset = range->cpu_addr - range->pci_addr; + return 0; +} + +static const struct of_device_id gen_pci_of_match[] = { + { .compatible = "pci-host-cam-generic", + .data = &gen_pci_cfg_cam_bus_ops }, + + { .compatible = "pci-host-ecam-generic", + .data = &gen_pci_cfg_ecam_bus_ops }, + + { }, +}; +MODULE_DEVICE_TABLE(of, gen_pci_of_match); + +static int gen_pci_setup(int nr, struct pci_sys_data *sys) +{ + int err, res_valid; + u8 bus_max; + struct pci_host_bridge_window *win; + struct resource *bus_range; + resource_size_t busn; + const char *type; + struct of_pci_range range; + struct of_pci_range_parser parser; + const struct of_device_id *of_id; + const int *prop; + struct gen_pci *pci = sys->private_data; + struct device *dev = pci->host.dev.parent; + struct device_node *np = dev->of_node; + + type = of_get_property(np, "device_type", NULL); + if (!type || strcmp(type, "pci")) { + dev_err(dev, "invalid \"device_type\" %s\n", type); + return -EINVAL; + } + + if (of_pci_range_parser_init(&parser, np)) { + dev_err(dev, "missing \"ranges\" property\n"); + return -EINVAL; + } + + prop = of_get_property(of_chosen, "linux,pci-probe-only", NULL); + if (prop) { + if (*prop) + pci_add_flags(PCI_PROBE_ONLY); + else + pci_clear_flags(PCI_PROBE_ONLY); + } + + if (of_pci_parse_bus_range(np, &pci->cfg.bus_range)) + pci->cfg.bus_range = (struct resource) { + .name = np->name, + .start = 0, + .end = 0xff, + .flags = IORESOURCE_BUS, + }; + + err = of_address_to_resource(np, 0, &pci->cfg.res); + if (err) { + dev_err(dev, "missing \"reg\" property\n"); + return err; + } + + pci->cfg.win = devm_kcalloc(dev, resource_size(&pci->cfg.bus_range), + sizeof(*pci->cfg.win), GFP_KERNEL); + if (!pci->cfg.win) + return -ENOMEM; + + of_id = of_match_node(gen_pci_of_match, np); + pci->cfg.ops = of_id->data; + INIT_LIST_HEAD(&pci->host.windows); + + /* Limit the bus-range to fit within reg */ + bus_max = pci->cfg.bus_range.start + + (resource_size(&pci->cfg.res) >> pci->cfg.ops->bus_shift) - 1; + pci->cfg.bus_range.end = min_t(resource_size_t, pci->cfg.bus_range.end, + bus_max); + + /* Create and register resources from the ranges property */ + res_valid = 0; + for_each_of_pci_range(&parser, &range) { + struct resource *parent, *res; + resource_size_t offset; + u32 restype = range.flags & IORESOURCE_TYPE_BITS; + + res = devm_kmalloc(dev, sizeof(*res), GFP_KERNEL); + if (!res) { + err = -ENOMEM; + goto out_release_res; + } + + switch (restype) { + case IORESOURCE_IO: + parent = &ioport_resource; + err = gen_pci_calc_io_offset(dev, &range, res, &offset); + break; + case IORESOURCE_MEM: + parent = &iomem_resource; + err = gen_pci_calc_mem_offset(dev, &range, res, &offset); + res_valid |= !(res->flags & IORESOURCE_PREFETCH || err); + break; + default: + err = -EINVAL; + continue; + } + + if (err) { + dev_warn(dev, + "error %d: failed to add resource [type 0x%x, %lld bytes]\n", + err, restype, range.size); + continue; + } + + err = request_resource(parent, res); + if (err) + goto out_release_res; + + pci_add_resource_offset(&sys->resources, res, offset); + } + + if (!res_valid) { + dev_err(dev, "non-prefetchable memory resource required\n"); + err = -EINVAL; + goto out_release_res; + } + + /* Map our Configuration Space windows */ + if (!request_mem_region(pci->cfg.res.start, + resource_size(&pci->cfg.res), + "Configuration Space")) + goto out_release_res; + + bus_range = &pci->cfg.bus_range; + for (busn = bus_range->start; busn <= bus_range->end; ++busn) { + u32 idx = busn - bus_range->start; + u32 sz = 1 << pci->cfg.ops->bus_shift; + + pci->cfg.win[idx] = devm_ioremap(dev, + pci->cfg.res.start + busn * sz, + sz); + if (!pci->cfg.win[idx]) { + err = -ENOMEM; + goto out_unmap_cfg; + } + } + + /* Register bus resource */ + pci_add_resource(&sys->resources, bus_range); + return 1; + +out_unmap_cfg: + while (busn-- > bus_range->start) + devm_iounmap(dev, pci->cfg.win[busn - bus_range->start]); + +out_release_res: + release_child_resources(&iomem_resource); + release_child_resources(&ioport_resource); + + list_for_each_entry(win, &sys->resources, list) + devm_kfree(dev, win->res); + pci_free_resource_list(&sys->resources); + + devm_kfree(dev, pci->cfg.win); + return err; +} + +static int gen_pci_probe(struct platform_device *pdev) +{ + struct hw_pci hw; + struct gen_pci *pci; + struct device *dev = &pdev->dev; + + if (!dev->of_node) + return -ENODEV; + + pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); + if (!pci) + return -ENOMEM; + + pci->host.dev.parent = dev; + hw = (struct hw_pci) { + .nr_controllers = 1, + .private_data = (void **)&pci, + .setup = gen_pci_setup, + .map_irq = of_irq_parse_and_map_pci, + .ops = &gen_pci_ops, + }; + + pci_common_init_dev(dev, &hw); + return 0; +} + +static struct platform_driver gen_pci_driver = { + .driver = { + .name = "pci-host-generic", + .owner = THIS_MODULE, + .of_match_table = gen_pci_of_match, + }, + .probe = gen_pci_probe, +}; +module_platform_driver(gen_pci_driver); + +MODULE_DESCRIPTION("Generic PCI host driver"); +MODULE_AUTHOR("Will Deacon <will.deacon@arm.com>"); +MODULE_LICENSE("GPLv2"); -- 1.9.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller 2014-05-02 16:41 ` [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller Will Deacon @ 2014-05-02 17:23 ` Jason Gunthorpe 2014-05-02 18:25 ` Arnd Bergmann 0 siblings, 1 reply; 15+ messages in thread From: Jason Gunthorpe @ 2014-05-02 17:23 UTC (permalink / raw) To: Will Deacon; +Cc: linux-arm-kernel, arnd, linux-pci, bhelgaas On Fri, May 02, 2014 at 05:41:15PM +0100, Will Deacon wrote: > + bus_range = &pci->cfg.bus_range; > + for (busn = bus_range->start; busn <= bus_range->end; ++busn) { > + u32 idx = busn - bus_range->start; > + u32 sz = 1 << pci->cfg.ops->bus_shift; > + > + pci->cfg.win[idx] = devm_ioremap(dev, > + pci->cfg.res.start + busn * sz, > + sz); Why map each bus individually? Both CAM and ECAM define consecutive busses consecutively in the address space, and I though ioremap was OK with unaligned stuff? > +out_unmap_cfg: > + while (busn-- > bus_range->start) > + devm_iounmap(dev, pci->cfg.win[busn - bus_range->start]); Is there a reason to explicitly clean up devm resources? I guess it is because this is in setup not probe? It seems strange to me for a driver to do this sort of work in a setup function, typically probe acquires as much stuff as it can, that way defered probe can work properly. Looking at pci-mvebu, 'setup' is only populating the resource list, I would suggest the same split for this driver. > +out_release_res: > + release_child_resources(&iomem_resource); > + release_child_resources(&ioport_resource); This looks really off to me, doesn't this free all resources in the system? This isn't enough?: > + list_for_each_entry(win, &sys->resources, list) > + devm_kfree(dev, win->res); > + pci_free_resource_list(&sys->resources); At worst, I would guess 'release_child_resources(win->res);' in the loop. But since no bus scan has been done, is there any chance of child resources at this point? Regards, Jason ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller 2014-05-02 17:23 ` Jason Gunthorpe @ 2014-05-02 18:25 ` Arnd Bergmann 2014-05-02 18:44 ` Will Deacon 0 siblings, 1 reply; 15+ messages in thread From: Arnd Bergmann @ 2014-05-02 18:25 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: Will Deacon, linux-arm-kernel, linux-pci, bhelgaas On Friday 02 May 2014 11:23:18 Jason Gunthorpe wrote: > On Fri, May 02, 2014 at 05:41:15PM +0100, Will Deacon wrote: > > > + bus_range = &pci->cfg.bus_range; > > + for (busn = bus_range->start; busn <= bus_range->end; ++busn) { > > + u32 idx = busn - bus_range->start; > > + u32 sz = 1 << pci->cfg.ops->bus_shift; > > + > > + pci->cfg.win[idx] = devm_ioremap(dev, > > + pci->cfg.res.start + busn * sz, > > + sz); > > Why map each bus individually? Both CAM and ECAM define consecutive > busses consecutively in the address space, and I though ioremap was OK > with unaligned stuff? One optimization we discussed before was to do this ioremap on the first access to any config space register in one bus, so we don't actually have to map all of them but only the ones that are in use. I don't know if there was a technical problem with that. We can't just map/unmap on every config space access, because it can be called from atomic context and ioremap can sleep, but the initial bus scan is always done in a context in which we are allowed to sleep. > > +out_unmap_cfg: > > + while (busn-- > bus_range->start) > > + devm_iounmap(dev, pci->cfg.win[busn - bus_range->start]); > > Is there a reason to explicitly clean up devm resources? I guess it is > because this is in setup not probe? Setup is called from probe, through pci_common_init_dev(), so that shouldn't make a difference. > It seems strange to me for a driver to do this sort of work in a setup > function, typically probe acquires as much stuff as it can, that way > defered probe can work properly. > > Looking at pci-mvebu, 'setup' is only populating the resource list, I > would suggest the same split for this driver. I suggested moving it all into setup, to make it easier to port this code to arm64: I don't expect we will have the same pci_common_init_dev() mechanism there, so setup will get called directly from the probe function. The alternative is to do everything in probe() as well for arm32, and only do a single list_move() of the resources list to sys->resources in the setup function. That was the advice I gave in the xilinx pci host driver review for the same reason. I only now noticed that I recommended the opposite here. Anyway it shouldn't matter where we do all the things, but I feel it's better to have only one function that does all the work for the case of having nr_controllers=1, as we always do for loadable host drivers. Arnd ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller 2014-05-02 18:25 ` Arnd Bergmann @ 2014-05-02 18:44 ` Will Deacon 2014-05-02 19:03 ` Jason Gunthorpe 0 siblings, 1 reply; 15+ messages in thread From: Will Deacon @ 2014-05-02 18:44 UTC (permalink / raw) To: Arnd Bergmann; +Cc: Jason Gunthorpe, linux-arm-kernel, linux-pci, bhelgaas Hi Arnd, Jason, Thanks for taking a look. On Fri, May 02, 2014 at 07:25:36PM +0100, Arnd Bergmann wrote: > On Friday 02 May 2014 11:23:18 Jason Gunthorpe wrote: > > On Fri, May 02, 2014 at 05:41:15PM +0100, Will Deacon wrote: > > > > > + bus_range = &pci->cfg.bus_range; > > > + for (busn = bus_range->start; busn <= bus_range->end; ++busn) { > > > + u32 idx = busn - bus_range->start; > > > + u32 sz = 1 << pci->cfg.ops->bus_shift; > > > + > > > + pci->cfg.win[idx] = devm_ioremap(dev, > > > + pci->cfg.res.start + busn * sz, > > > + sz); > > > > Why map each bus individually? Both CAM and ECAM define consecutive > > busses consecutively in the address space, and I though ioremap was OK > > with unaligned stuff? > > One optimization we discussed before was to do this ioremap on the first > access to any config space register in one bus, so we don't actually have > to map all of them but only the ones that are in use. Right, and this optimisation is because we don't have a lot of virtual address space on 32-bit ARM, so blowing away 256M on ECAM isn't viable. > I don't know if there was a technical problem with that. We can't just > map/unmap on every config space access, because it can be called from > atomic context and ioremap can sleep, but the initial bus scan is > always done in a context in which we are allowed to sleep. It just doesn't seem worth it, given that we have the bus-range property in the DT. I can revisit it if there are strong objections to the current code though (looking back, I ended up needing to take a lock last time I tried this). > > > +out_unmap_cfg: > > > + while (busn-- > bus_range->start) > > > + devm_iounmap(dev, pci->cfg.win[busn - bus_range->start]); > > > > Is there a reason to explicitly clean up devm resources? I guess it is > > because this is in setup not probe? > > Setup is called from probe, through pci_common_init_dev(), so that shouldn't > make a difference. Given that the idea was to separate setup() and probe(), I didn't want to make the assumption that I was called in probe context. > > It seems strange to me for a driver to do this sort of work in a setup > > function, typically probe acquires as much stuff as it can, that way > > defered probe can work properly. > > > > Looking at pci-mvebu, 'setup' is only populating the resource list, I > > would suggest the same split for this driver. > > I suggested moving it all into setup, to make it easier to port this code > to arm64: I don't expect we will have the same pci_common_init_dev() > mechanism there, so setup will get called directly from the probe > function. In which case, I *could* remove that freeing code, but I'd rather wait until we know for sure that it's not needed (that is, when I go about plumbing in the support for arm64 after Liviu's patches are merged). > The alternative is to do everything in probe() as well for arm32, and only > do a single list_move() of the resources list to sys->resources in the > setup function. That was the advice I gave in the xilinx pci host driver > review for the same reason. I only now noticed that I recommended the opposite > here. Anyway it shouldn't matter where we do all the things, but I feel > it's better to have only one function that does all the work for the case > of having nr_controllers=1, as we always do for loadable host drivers. Ok, I'll leave it like it is for now then. Will ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller 2014-05-02 18:44 ` Will Deacon @ 2014-05-02 19:03 ` Jason Gunthorpe 2014-05-02 19:29 ` Arnd Bergmann 2014-05-06 16:05 ` Will Deacon 0 siblings, 2 replies; 15+ messages in thread From: Jason Gunthorpe @ 2014-05-02 19:03 UTC (permalink / raw) To: Will Deacon; +Cc: Arnd Bergmann, linux-arm-kernel, linux-pci, bhelgaas On Fri, May 02, 2014 at 07:44:21PM +0100, Will Deacon wrote: > > > > + bus_range = &pci->cfg.bus_range; > > > > + for (busn = bus_range->start; busn <= bus_range->end; ++busn) { > > > > + u32 idx = busn - bus_range->start; > > > > + u32 sz = 1 << pci->cfg.ops->bus_shift; > > > > + > > > > + pci->cfg.win[idx] = devm_ioremap(dev, > > > > + pci->cfg.res.start + busn * sz, > > > > + sz); > > > > > > Why map each bus individually? Both CAM and ECAM define consecutive > > > busses consecutively in the address space, and I though ioremap was OK > > > with unaligned stuff? > > > > One optimization we discussed before was to do this ioremap on the first > > access to any config space register in one bus, so we don't actually have > > to map all of them but only the ones that are in use. > > Right, and this optimisation is because we don't have a lot of virtual > address space on 32-bit ARM, so blowing away 256M on ECAM isn't viable. But why not just devm_ioremap(dev, pci->cfg.res.start + bus_range->start * sz, resource_size(&bus_range) * sz); ? > > Setup is called from probe, through pci_common_init_dev(), so that shouldn't > > make a difference. > > Given that the idea was to separate setup() and probe(), I didn't want to > make the assumption that I was called in probe context. IMHO, we need to have clear purposes for setup() and probe(). Probe is well defined already, it requests resources, claims the device, puts it in reset and then does some subsystem specific thing. The interaction with probe and devm is also already well specified. It doesn't matter for this driver, but look at mvebu, you cannot move the interrupt, gpio and clock acquisitions from probe() to setup(), as they could all trigger a defered probe. Better to be consistent, especially if this is the golden reference driver we want everyone to follow (sorry Will) To me setup() is more like netdev open(), so it should just do that final step to enable the links and bring up the PCI network and be ready to run PCI discovery. Consider, someday we might have an unsetup() for power mangement reasons, just like we have a close() in netdev. If the long term plan is to keep probe() then I don't think it makes sense to move probe() duties into setup(). That just restricts what we can do with the setup() call if setup() is now required to always handle defered probe and so forth. Will, don't forget this q: ------ > +out_release_res: > + release_child_resources(&iomem_resource); > + release_child_resources(&ioport_resource); This looks really off to me, doesn't this free all resources in the system? ------- Regards, Jason ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller 2014-05-02 19:03 ` Jason Gunthorpe @ 2014-05-02 19:29 ` Arnd Bergmann 2014-05-06 18:38 ` Will Deacon 2014-05-06 16:05 ` Will Deacon 1 sibling, 1 reply; 15+ messages in thread From: Arnd Bergmann @ 2014-05-02 19:29 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: Will Deacon, linux-arm-kernel, linux-pci, bhelgaas On Friday 02 May 2014 13:03:18 Jason Gunthorpe wrote: > > > > Setup is called from probe, through pci_common_init_dev(), so that shouldn't > > > make a difference. > > > > Given that the idea was to separate setup() and probe(), I didn't want to > > make the assumption that I was called in probe context. > > IMHO, we need to have clear purposes for setup() and probe(). > > Probe is well defined already, it requests resources, claims the > device, puts it in reset and then does some subsystem specific thing. > The interaction with probe and devm is also already well specified. > > It doesn't matter for this driver, but look at mvebu, you cannot move > the interrupt, gpio and clock acquisitions from probe() to setup(), as > they could all trigger a defered probe. Better to be consistent, > especially if this is the golden reference driver we want everyone to > follow (sorry Will) Fair enough. It shouldn't be hard to move. > To me setup() is more like netdev open(), so it should just do that > final step to enable the links and bring up the PCI network and be > ready to run PCI discovery. Consider, someday we might have an > unsetup() for power mangement reasons, just like we have a close() in > netdev. I expect setup() to just go away in the long run for loadable host controllers, and get merged into probe(). > If the long term plan is to keep probe() then I don't think it makes > sense to move probe() duties into setup(). That just restricts what we > can do with the setup() call if setup() is now required to always > handle defered probe and so forth. setup() makes sense for the traditional ARM32 PCI support with nr_controllers > 1: dove, kirkwood, mv78xx0, orion5x and iop13xx (most of these are gradually getting replace with pci-mvebu.c, which doesn't do that, as you know). All other classic drivers only support one host bridge anyway, and the loadable driver just use one domain per host bridge, which means you lose all the advantages of having pci_common_init_dev() assign the bus numbers and call setup functions for each host bridge. It should be fine to have a trivial setup function like this static int gen_pci_setup(int nr, struct pci_sys_data *sys) { struct gen_pci *pci = sys->private_data; list_splice_init(pci->resource, sys->resources); return 1; } and do everything in probe(). Arnd ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller 2014-05-02 19:29 ` Arnd Bergmann @ 2014-05-06 18:38 ` Will Deacon 2014-05-06 19:11 ` Arnd Bergmann 0 siblings, 1 reply; 15+ messages in thread From: Will Deacon @ 2014-05-06 18:38 UTC (permalink / raw) To: Arnd Bergmann; +Cc: Jason Gunthorpe, linux-arm-kernel, linux-pci, bhelgaas On Fri, May 02, 2014 at 08:29:30PM +0100, Arnd Bergmann wrote: > On Friday 02 May 2014 13:03:18 Jason Gunthorpe wrote: > > > Given that the idea was to separate setup() and probe(), I didn't want to > > > make the assumption that I was called in probe context. > > > > It doesn't matter for this driver, but look at mvebu, you cannot move > > the interrupt, gpio and clock acquisitions from probe() to setup(), as > > they could all trigger a defered probe. Better to be consistent, > > especially if this is the golden reference driver we want everyone to > > follow (sorry Will) > > Fair enough. It shouldn't be hard to move. [...] > It should be fine to have a trivial setup function like this > > static int gen_pci_setup(int nr, struct pci_sys_data *sys) > { > struct gen_pci *pci = sys->private_data; > list_splice_init(pci->resource, sys->resources); > return 1; > } > > and do everything in probe(). Ok, I've respun the patch and included it below. It's turned probe() into a bit of a beast, but the cleanup is a lot simpler (unless I'm missing something). Will --->8 diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c new file mode 100644 index 000000000000..199a8795cb77 --- /dev/null +++ b/drivers/pci/host/pci-host-generic.c @@ -0,0 +1,361 @@ +/* + * Simple, generic PCI host controller driver targetting firmware-initialised + * systems and virtual machines (e.g. the PCI emulation provided by kvmtool). + * + * 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. + * + * 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 for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + * + * Copyright (C) 2014 ARM Limited + * + * Author: Will Deacon <will.deacon@arm.com> + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/of_pci.h> +#include <linux/platform_device.h> + +struct gen_pci_cfg_bus_ops { + u32 bus_shift; + void __iomem *(*map_bus)(struct pci_bus *, unsigned int, int); +}; + +struct gen_pci_cfg_windows { + struct resource res; + struct resource bus_range; + void __iomem **win; + + const struct gen_pci_cfg_bus_ops *ops; +}; + +struct gen_pci { + struct pci_host_bridge host; + struct gen_pci_cfg_windows cfg; + struct list_head resources; +}; + +static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus, + unsigned int devfn, + int where) +{ + struct pci_sys_data *sys = bus->sysdata; + struct gen_pci *pci = sys->private_data; + resource_size_t idx = bus->number - pci->cfg.bus_range.start; + + return pci->cfg.win[idx] + ((devfn << 8) | where); +} + +static struct gen_pci_cfg_bus_ops gen_pci_cfg_cam_bus_ops = { + .bus_shift = 16, + .map_bus = gen_pci_map_cfg_bus_cam, +}; + +static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus, + unsigned int devfn, + int where) +{ + struct pci_sys_data *sys = bus->sysdata; + struct gen_pci *pci = sys->private_data; + resource_size_t idx = bus->number - pci->cfg.bus_range.start; + + return pci->cfg.win[idx] + ((devfn << 12) | where); +} + +static struct gen_pci_cfg_bus_ops gen_pci_cfg_ecam_bus_ops = { + .bus_shift = 20, + .map_bus = gen_pci_map_cfg_bus_ecam, +}; + +static int gen_pci_config_read(struct pci_bus *bus, unsigned int devfn, + int where, int size, u32 *val) +{ + void __iomem *addr; + struct pci_sys_data *sys = bus->sysdata; + struct gen_pci *pci = sys->private_data; + + addr = pci->cfg.ops->map_bus(bus, devfn, where); + + switch (size) { + case 1: + *val = readb(addr); + break; + case 2: + *val = readw(addr); + break; + default: + *val = readl(addr); + } + + return PCIBIOS_SUCCESSFUL; +} + +static int gen_pci_config_write(struct pci_bus *bus, unsigned int devfn, + int where, int size, u32 val) +{ + void __iomem *addr; + struct pci_sys_data *sys = bus->sysdata; + struct gen_pci *pci = sys->private_data; + + addr = pci->cfg.ops->map_bus(bus, devfn, where); + + switch (size) { + case 1: + writeb(val, addr); + break; + case 2: + writew(val, addr); + break; + default: + writel(val, addr); + } + + return PCIBIOS_SUCCESSFUL; +} + +static struct pci_ops gen_pci_ops = { + .read = gen_pci_config_read, + .write = gen_pci_config_write, +}; + +static int gen_pci_calc_io_offset(struct device *dev, + struct of_pci_range *range, + struct resource *res, + resource_size_t *offset) +{ + static atomic_t wins = ATOMIC_INIT(0); + int err, idx, max_win; + unsigned int window; + + if (!PAGE_ALIGNED(range->cpu_addr)) + return -EINVAL; + + max_win = (IO_SPACE_LIMIT + 1) / SZ_64K; + idx = atomic_inc_return(&wins); + if (idx >= max_win) + return -ENOSPC; + + window = (idx - 1) * SZ_64K; + err = pci_ioremap_io(window, range->cpu_addr); + if (err) + return err; + + of_pci_range_to_resource(range, dev->of_node, res); + res->start = window; + res->end = res->start + range->size - 1; + *offset = window - range->pci_addr; + return 0; +} + +static int gen_pci_calc_mem_offset(struct device *dev, + struct of_pci_range *range, + struct resource *res, + resource_size_t *offset) +{ + of_pci_range_to_resource(range, dev->of_node, res); + *offset = range->cpu_addr - range->pci_addr; + return 0; +} + +static const struct of_device_id gen_pci_of_match[] = { + { .compatible = "pci-host-cam-generic", + .data = &gen_pci_cfg_cam_bus_ops }, + + { .compatible = "pci-host-ecam-generic", + .data = &gen_pci_cfg_ecam_bus_ops }, + + { }, +}; +MODULE_DEVICE_TABLE(of, gen_pci_of_match); + +static int gen_pci_setup(int nr, struct pci_sys_data *sys) +{ + struct gen_pci *pci = sys->private_data; + list_splice_init(&pci->resources, &sys->resources); + return 1; +} + +static int gen_pci_probe(struct platform_device *pdev) +{ + struct gen_pci *pci; + int err, res_valid; + struct pci_host_bridge_window *win; + struct resource *bus_range; + resource_size_t busn; + const char *type; + struct of_pci_range range; + struct of_pci_range_parser parser; + struct hw_pci hw; + const struct of_device_id *of_id; + const int *prop; + u8 bus_max; + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + + if (!dev->of_node) + return -ENODEV; + + pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); + if (!pci) + return -ENOMEM; + + pci->host.dev.parent = dev; + hw = (struct hw_pci) { + .nr_controllers = 1, + .private_data = (void **)&pci, + .setup = gen_pci_setup, + .map_irq = of_irq_parse_and_map_pci, + .ops = &gen_pci_ops, + }; + + type = of_get_property(np, "device_type", NULL); + if (!type || strcmp(type, "pci")) { + dev_err(dev, "invalid \"device_type\" %s\n", type); + return -EINVAL; + } + + if (of_pci_range_parser_init(&parser, np)) { + dev_err(dev, "missing \"ranges\" property\n"); + return -EINVAL; + } + + prop = of_get_property(of_chosen, "linux,pci-probe-only", NULL); + if (prop) { + if (*prop) + pci_add_flags(PCI_PROBE_ONLY); + else + pci_clear_flags(PCI_PROBE_ONLY); + } + + if (of_pci_parse_bus_range(np, &pci->cfg.bus_range)) + pci->cfg.bus_range = (struct resource) { + .name = np->name, + .start = 0, + .end = 0xff, + .flags = IORESOURCE_BUS, + }; + + err = of_address_to_resource(np, 0, &pci->cfg.res); + if (err) { + dev_err(dev, "missing \"reg\" property\n"); + return err; + } + + pci->cfg.win = devm_kcalloc(dev, resource_size(&pci->cfg.bus_range), + sizeof(*pci->cfg.win), GFP_KERNEL); + if (!pci->cfg.win) + return -ENOMEM; + + of_id = of_match_node(gen_pci_of_match, np); + pci->cfg.ops = of_id->data; + INIT_LIST_HEAD(&pci->host.windows); + INIT_LIST_HEAD(&pci->resources); + + /* Limit the bus-range to fit within reg */ + bus_max = pci->cfg.bus_range.start + + (resource_size(&pci->cfg.res) >> pci->cfg.ops->bus_shift) - 1; + pci->cfg.bus_range.end = min_t(resource_size_t, pci->cfg.bus_range.end, + bus_max); + + /* Create and register resources from the ranges property */ + res_valid = 0; + for_each_of_pci_range(&parser, &range) { + struct resource *parent, *res; + resource_size_t offset; + u32 restype = range.flags & IORESOURCE_TYPE_BITS; + + res = devm_kmalloc(dev, sizeof(*res), GFP_KERNEL); + if (!res) { + err = -ENOMEM; + goto out_release_res; + } + + switch (restype) { + case IORESOURCE_IO: + parent = &ioport_resource; + err = gen_pci_calc_io_offset(dev, &range, res, &offset); + break; + case IORESOURCE_MEM: + parent = &iomem_resource; + err = gen_pci_calc_mem_offset(dev, &range, res, &offset); + res_valid |= !(res->flags & IORESOURCE_PREFETCH || err); + break; + default: + err = -EINVAL; + continue; + } + + if (err) { + dev_warn(dev, + "error %d: failed to add resource [type 0x%x, %lld bytes]\n", + err, restype, range.size); + continue; + } + + err = request_resource(parent, res); + if (err) + goto out_release_res; + + pci_add_resource_offset(&pci->resources, res, offset); + } + + if (!res_valid) { + dev_err(dev, "non-prefetchable memory resource required\n"); + err = -EINVAL; + goto out_release_res; + } + + /* Map our Configuration Space windows */ + err = -ENOMEM; + if (!devm_request_mem_region(dev, pci->cfg.res.start, + resource_size(&pci->cfg.res), + "Configuration Space")) + goto out_release_res; + + bus_range = &pci->cfg.bus_range; + for (busn = bus_range->start; busn <= bus_range->end; ++busn) { + u32 idx = busn - bus_range->start; + u32 sz = 1 << pci->cfg.ops->bus_shift; + + pci->cfg.win[idx] = devm_ioremap(dev, + pci->cfg.res.start + busn * sz, + sz); + if (!pci->cfg.win[idx]) + goto out_release_res; + } + + /* Register bus resource */ + pci_add_resource(&pci->resources, bus_range); + pci_common_init_dev(dev, &hw); + return 0; + +out_release_res: + list_for_each_entry(win, &pci->resources, list) + release_resource(win->res); + + pci_free_resource_list(&pci->resources); + return err; +} + +static struct platform_driver gen_pci_driver = { + .driver = { + .name = "pci-host-generic", + .owner = THIS_MODULE, + .of_match_table = gen_pci_of_match, + }, + .probe = gen_pci_probe, +}; +module_platform_driver(gen_pci_driver); + +MODULE_DESCRIPTION("Generic PCI host driver"); +MODULE_AUTHOR("Will Deacon <will.deacon@arm.com>"); +MODULE_LICENSE("GPLv2"); ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller 2014-05-06 18:38 ` Will Deacon @ 2014-05-06 19:11 ` Arnd Bergmann 2014-05-07 9:18 ` Will Deacon 0 siblings, 1 reply; 15+ messages in thread From: Arnd Bergmann @ 2014-05-06 19:11 UTC (permalink / raw) To: Will Deacon; +Cc: Jason Gunthorpe, linux-arm-kernel, linux-pci, bhelgaas On Tuesday 06 May 2014 19:38:12 Will Deacon wrote: > Ok, I've respun the patch and included it below. It's turned probe() into a > bit of a beast, but the cleanup is a lot simpler (unless I'm missing > something). Let's see if we can make it a bit more readable. None of these are important, but together they could improve readability. > +static int gen_pci_probe(struct platform_device *pdev) > +{ > + struct gen_pci *pci; > + int err, res_valid; > + struct pci_host_bridge_window *win; > + struct resource *bus_range; > + resource_size_t busn; > + const char *type; > + struct of_pci_range range; > + struct of_pci_range_parser parser; > + struct hw_pci hw; > + const struct of_device_id *of_id; > + const int *prop; > + u8 bus_max; > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + > + if (!dev->of_node) > + return -ENODEV; This check can go away: you don't get here without an of node. > + pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); > + if (!pci) > + return -ENOMEM; > + > + pci->host.dev.parent = dev; > + hw = (struct hw_pci) { > + .nr_controllers = 1, > + .private_data = (void **)&pci, > + .setup = gen_pci_setup, > + .map_irq = of_irq_parse_and_map_pci, > + .ops = &gen_pci_ops, > + }; C constructors are actually a gcc extension, I think it would be more common to turn this into an initializer by moving it to the declaration: struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); struct hw_pci hw = { .nr_controllers = 1, .private_data = (void **)&pci, .setup = gen_pci_setup, .map_irq = of_irq_parse_and_map_pci, .ops = &gen_pci_ops, }; > + if (of_pci_range_parser_init(&parser, np)) { > + dev_err(dev, "missing \"ranges\" property\n"); > + return -EINVAL; > + } If you move everything related to the range parser into a separate function, you end up with two more compact parts. > + /* Create and register resources from the ranges property */ > + res_valid = 0; > + for_each_of_pci_range(&parser, &range) { > + struct resource *parent, *res; > + resource_size_t offset; > + u32 restype = range.flags & IORESOURCE_TYPE_BITS; You could also move the body of the for loop into another function, or both. Arnd ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller 2014-05-06 19:11 ` Arnd Bergmann @ 2014-05-07 9:18 ` Will Deacon 0 siblings, 0 replies; 15+ messages in thread From: Will Deacon @ 2014-05-07 9:18 UTC (permalink / raw) To: Arnd Bergmann; +Cc: Jason Gunthorpe, linux-arm-kernel, linux-pci, bhelgaas Hi Arnd, On Tue, May 06, 2014 at 08:11:48PM +0100, Arnd Bergmann wrote: > On Tuesday 06 May 2014 19:38:12 Will Deacon wrote: > > Ok, I've respun the patch and included it below. It's turned probe() into a > > bit of a beast, but the cleanup is a lot simpler (unless I'm missing > > something). > > Let's see if we can make it a bit more readable. None of these are important, > but together they could improve readability. Sure, I managed to incorporate most of your ideas, but ended up splitting probe into two other functions; one for dealing with the ranges and the other for dealing with config space. I'll send a v6 later on. Cheers, Will ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller 2014-05-02 19:03 ` Jason Gunthorpe 2014-05-02 19:29 ` Arnd Bergmann @ 2014-05-06 16:05 ` Will Deacon 1 sibling, 0 replies; 15+ messages in thread From: Will Deacon @ 2014-05-06 16:05 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: Arnd Bergmann, linux-arm-kernel, linux-pci, bhelgaas On Fri, May 02, 2014 at 08:03:18PM +0100, Jason Gunthorpe wrote: > On Fri, May 02, 2014 at 07:44:21PM +0100, Will Deacon wrote: > > > > > + bus_range = &pci->cfg.bus_range; > > > > > + for (busn = bus_range->start; busn <= bus_range->end; ++busn) { > > > > > + u32 idx = busn - bus_range->start; > > > > > + u32 sz = 1 << pci->cfg.ops->bus_shift; > > > > > + > > > > > + pci->cfg.win[idx] = devm_ioremap(dev, > > > > > + pci->cfg.res.start + busn * sz, > > > > > + sz); > > > > > > > > Why map each bus individually? Both CAM and ECAM define consecutive > > > > busses consecutively in the address space, and I though ioremap was OK > > > > with unaligned stuff? > > > > > > One optimization we discussed before was to do this ioremap on the first > > > access to any config space register in one bus, so we don't actually have > > > to map all of them but only the ones that are in use. > > > > Right, and this optimisation is because we don't have a lot of virtual > > address space on 32-bit ARM, so blowing away 256M on ECAM isn't viable. > > But why not just > > devm_ioremap(dev, pci->cfg.res.start + bus_range->start * sz, > resource_size(&bus_range) * sz); > > ? I was just trying to avoid the need for a single, virtually contiguous range where it's not actually required. If the driver is loaded as a module, we could already have a bunch of fragmentation in the vmalloc space. > > > Setup is called from probe, through pci_common_init_dev(), so that shouldn't > > > make a difference. > > > > Given that the idea was to separate setup() and probe(), I didn't want to > > make the assumption that I was called in probe context. > > IMHO, we need to have clear purposes for setup() and probe(). > > Probe is well defined already, it requests resources, claims the > device, puts it in reset and then does some subsystem specific thing. > The interaction with probe and devm is also already well specified. > > It doesn't matter for this driver, but look at mvebu, you cannot move > the interrupt, gpio and clock acquisitions from probe() to setup(), as > they could all trigger a defered probe. Better to be consistent, > especially if this is the golden reference driver we want everyone to > follow (sorry Will) Groan! > To me setup() is more like netdev open(), so it should just do that > final step to enable the links and bring up the PCI network and be > ready to run PCI discovery. Consider, someday we might have an > unsetup() for power mangement reasons, just like we have a close() in > netdev. That's what I did originally! Anyway, this is just refactoring so shouldn't be too much work. After reading Arnd's reply, I'll move the bulk into ->probe(). > If the long term plan is to keep probe() then I don't think it makes > sense to move probe() duties into setup(). That just restricts what we > can do with the setup() call if setup() is now required to always > handle defered probe and so forth. > > Will, don't forget this q: Sorry, that got chopped off somehow... > ------ > > +out_release_res: > > + release_child_resources(&iomem_resource); > > + release_child_resources(&ioport_resource); > > This looks really off to me, doesn't this free all resources in the > system? ... yes, I suppose that would cause problems if we have multiple instances of the driver. I could probably just release_resource(win->res) instead of freeing the resources and use devm_request_mem_region for configuration space. Will ^ permalink raw reply [flat|nested] 15+ messages in thread
* [RESEND PATCH 3/3] MAINTAINERS: add entry for generic PCI host controller driver 2014-05-02 16:41 [RESEND PATCH 0/3] Support for Generic PCI Host Controller Will Deacon 2014-05-02 16:41 ` [RESEND PATCH 1/3] ARM: mach-virt: allow PCI support to be selected Will Deacon 2014-05-02 16:41 ` [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller Will Deacon @ 2014-05-02 16:41 ` Will Deacon 2 siblings, 0 replies; 15+ messages in thread From: Will Deacon @ 2014-05-02 16:41 UTC (permalink / raw) To: linux-arm-kernel; +Cc: arnd, linux-pci, bhelgaas, jgunthorpe, Will Deacon Add myself as the maintainer for the generic PCI host controller driver. Cc: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Will Deacon <will.deacon@arm.com> --- MAINTAINERS | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index e67ea2442041..d5db11484062 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6740,6 +6740,14 @@ L: linux-pci@vger.kernel.org S: Maintained F: drivers/pci/host/*designware* +PCI DRIVER FOR GENERIC OF HOSTS +M: Will Deacon <will.deacon@arm.com> +L: linux-pci@vger.kernel.org +L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers) +S: Maintained +F: Documentation/devicetree/bindings/pci/host-generic-pci.txt +F: drivers/pci/host/pci-host-generic.c + PCMCIA SUBSYSTEM P: Linux PCMCIA Team L: linux-pcmcia@lists.infradead.org -- 1.9.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2014-05-07 9:18 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-05-02 16:41 [RESEND PATCH 0/3] Support for Generic PCI Host Controller Will Deacon 2014-05-02 16:41 ` [RESEND PATCH 1/3] ARM: mach-virt: allow PCI support to be selected Will Deacon 2014-05-02 18:11 ` Arnd Bergmann 2014-05-02 18:21 ` Will Deacon 2014-05-02 16:41 ` [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller Will Deacon 2014-05-02 17:23 ` Jason Gunthorpe 2014-05-02 18:25 ` Arnd Bergmann 2014-05-02 18:44 ` Will Deacon 2014-05-02 19:03 ` Jason Gunthorpe 2014-05-02 19:29 ` Arnd Bergmann 2014-05-06 18:38 ` Will Deacon 2014-05-06 19:11 ` Arnd Bergmann 2014-05-07 9:18 ` Will Deacon 2014-05-06 16:05 ` Will Deacon 2014-05-02 16:41 ` [RESEND PATCH 3/3] MAINTAINERS: add entry for generic PCI host controller driver Will Deacon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).