lkml.org 
[lkml]   [2016]   [Apr]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH V6 07/13] PCI: Provide common functions for ECAM mapping
    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
    understand correctly, this infrastructure supports non-generic ECAM
    implementations as well, since the caller supplies "struct
    pci_generic_ecam_ops *ops".

    > 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.

    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.

    > + 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.

    > +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.

    > + 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 = { ... ?

    > + .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"

    > + 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
    > --
    > 1.9.1
    >
    > --
    > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
    > the body of a message to majordomo@vger.kernel.org
    > More majordomo info at http://vger.kernel.org/majordomo-info.html

    \
     
     \ /
      Last update: 2016-04-29 00:01    [W:5.380 / U:2.144 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site