[PATCH V6 07/13] PCI: Provide common functions for ECAM mapping
Tomasz Nowicki
tn at semihalf.com
Thu May 5 03:38:54 PDT 2016
On 05.05.2016 11:24, Jayachandran C wrote:
> On Fri, Apr 29, 2016 at 1:31 PM, Jayachandran C <jchandra at broadcom.com> wrote:
>> On Fri, Apr 29, 2016 at 3:17 AM, Bjorn Helgaas <helgaas at kernel.org> wrote:
>>> On Fri, Apr 15, 2016 at 07:06:42PM +0200, Tomasz Nowicki wrote:
>>>> From: Jayachandran C <jchandra at 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 at 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
More information about the linux-arm-kernel
mailing list