Re: [PATCH V6 09/13] pci, acpi: Support for ACPI based generic PCI host controller

[Jayachandran C]

On Fri, Apr 15, 2016 at 10:36 PM, Tomasz Nowicki <tn@xxxxxxxxxxxx> wrote:
> This patch is going to implement generic PCI host controller for
> ACPI world, similar to what pci-host-generic.c driver does for DT world.
>
> All such drivers, which we have seen so far, were implemented within
> arch/ directory since they had some arch assumptions (x86 and ia64).
> However, they all are doing similar thing, so it makes sense to find
> some common code and abstract it into the generic driver.
>
> In order to handle PCI config space regions properly, we define new
> MCFG interface which parses MCFG table and keep its entries
> in a list. New pci_mcfg_init call is defined so that we do not depend
> on PCI_MMCONFIG. Regions are not mapped until host bridge ask for it.
>
> The implementation of pci_acpi_scan_root() looks up the saved MCFG entries
> and sets up a new mapping. Generic PCI functions are used for
> accessing config space. Driver selects PCI_GENERIC_ECAM and uses functions
> from drivers/pci/ecam.h to create and access ECAM mappings.
>
> As mentioned in Kconfig help section, ACPI_PCI_HOST_GENERIC choice
> should be made on a per-architecture basis.
>
> This patch is heavily based on the updated version from Jayachandran C:
> https://lkml.org/lkml/2016/4/11/908
> git: https://github.com/jchandra-brcm/linux/ (arm64-acpi-pci-v3)

This is a little bit unusual because I had not posted the v3 patch
to the mailing list yet, but you posted a variant of it The git repository
should not be in the commit comment because it is a temporary location.

There are some changes here I don't agree with. I think it will be
better if you can post a version without the quirk handling and with
some of the suggestions below.

> Signed-off-by: Tomasz Nowicki <tn@xxxxxxxxxxxx>
> Signed-off-by: Jayachandran C <jchandra@xxxxxxxxxxxx>
> ---
>  drivers/acpi/Kconfig        |   8 ++
>  drivers/acpi/Makefile       |   1 +
>  drivers/acpi/bus.c          |   1 +
>  drivers/acpi/pci_gen_host.c | 231 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h         |   6 ++
>  5 files changed, 247 insertions(+)
>  create mode 100644 drivers/acpi/pci_gen_host.c
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 183ffa3..70272c5 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -346,6 +346,14 @@ config ACPI_PCI_SLOT
>           i.e., segment/bus/device/function tuples, with physical slots in
>           the system.  If you are unsure, say N.
>
> +config ACPI_PCI_HOST_GENERIC
> +       bool
> +       select PCI_GENERIC_ECAM
> +       help
> +         Select this config option from the architecture Kconfig,
> +         if it is preferred to enable ACPI PCI host controller driver which
> +         has no arch-specific assumptions.
> +
>  config X86_PM_TIMER
>         bool "Power Management Timer Support" if EXPERT
>         depends on X86
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 81e5cbc..b12fa64 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -40,6 +40,7 @@ acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
>  acpi-y                         += ec.o
>  acpi-$(CONFIG_ACPI_DOCK)       += dock.o
>  acpi-y                         += pci_root.o pci_link.o pci_irq.o
> +obj-$(CONFIG_ACPI_PCI_HOST_GENERIC)    += pci_gen_host.o
>  acpi-y                         += acpi_lpss.o acpi_apd.o
>  acpi-y                         += acpi_platform.o
>  acpi-y                         += acpi_pnp.o
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index c068c82..803a1d7 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -1107,6 +1107,7 @@ static int __init acpi_init(void)
>         }
>
>         pci_mmcfg_late_init();
> +       pci_mcfg_init();

Please see below.

>         acpi_scan_init();
>         acpi_ec_init();
>         acpi_debugfs_init();
> diff --git a/drivers/acpi/pci_gen_host.c b/drivers/acpi/pci_gen_host.c
> new file mode 100644
> index 0000000..fd360b5
> --- /dev/null
> +++ b/drivers/acpi/pci_gen_host.c
> @@ -0,0 +1,231 @@
> +/*

You seem to have removed the copyright line, this is not proper, you
should probably add your copyright line if you think your changes are
significant.

> + * 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/kernel.h>
> +#include <linux/pci.h>
> +#include <linux/pci-acpi.h>
> +#include <linux/sfi_acpi.h>
> +#include <linux/slab.h>
> +
> +#include "../pci/ecam.h"
> +
> +#define PREFIX "ACPI: "
> +
> +/* Structure to hold entries from the MCFG table */
> +struct mcfg_entry {
> +       struct list_head        list;
> +       phys_addr_t             addr;
> +       u16                     segment;
> +       u8                      bus_start;
> +       u8                      bus_end;
> +};
> +
> +/* List to save mcfg entries */
> +static LIST_HEAD(pci_mcfg_list);
> +static DEFINE_MUTEX(pci_mcfg_lock);

There is no need to use a list or lock here, I had used an
array and that is sufficient since it is not modified after it
is filled initially.

> +/* ACPI info for generic ACPI PCI controller */
> +struct acpi_pci_generic_root_info {
> +       struct acpi_pci_root_info       common;
> +       struct pci_config_window        *cfg;   /* config space mapping */
> +};
> +
> +/* Find the entry in mcfg list which contains range bus_start */
> +static struct mcfg_entry *pci_mcfg_lookup(u16 seg, u8 bus_start)
> +{
> +       struct mcfg_entry *e;
> +
> +       list_for_each_entry(e, &pci_mcfg_list, list) {
> +               if (e->segment == seg &&
> +                   e->bus_start <= bus_start && bus_start <= e->bus_end)
> +                       return e;
> +       }
> +
> +       return NULL;
> +}
> +
> +
> +/*
> + * Lookup the bus range for the domain in MCFG, and set up config space
> + * mapping.
> + */
> +static int pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root,
> +                                      struct acpi_pci_generic_root_info *ri)
> +{
> +       u16 seg = root->segment;
> +       u8 bus_start = root->secondary.start;
> +       u8 bus_end = root->secondary.end;
> +       struct pci_config_window *cfg;
> +       struct mcfg_entry *e;
> +       phys_addr_t addr;
> +       int err = 0;
> +
> +       mutex_lock(&pci_mcfg_lock);
> +       e = pci_mcfg_lookup(seg, bus_start);
> +       if (!e) {
> +               addr = acpi_pci_root_get_mcfg_addr(root->device->handle);

The acpi_pci_root_get_mcfg_addr() is already called in pci_root.c, doing
it again here is unnecessary.

I think you can have a function to pick up addr, bus_start, bus_end given
a domain from either MCFG or using _CBA method, but I think that
should be done in pci_root.c in a separate patch.

> +               if (addr == 0) {
> +                       pr_err(PREFIX"%04x:%02x-%02x bus range error\n",
> +                              seg, bus_start, bus_end);
> +                       err = -ENOENT;
> +                       goto err_out;
> +               }
> +       } else {
> +               if (bus_start != e->bus_start) {
> +                       pr_err("%04x:%02x-%02x bus range mismatch %02x\n",
> +                              seg, bus_start, bus_end, e->bus_start);
> +                       err = -EINVAL;
> +                       goto err_out;
> +               } else if (bus_end != e->bus_end) {
> +                       pr_warn("%04x:%02x-%02x bus end mismatch %02x\n",
> +                               seg, bus_start, bus_end, e->bus_end);
> +                       bus_end = min(bus_end, e->bus_end);
> +               }
> +               addr = e->addr;
> +       }
> +
> +       cfg = pci_generic_ecam_create(&root->device->dev, addr, bus_start,
> +                                     bus_end, &pci_generic_ecam_default_ops);
> +       if (IS_ERR(cfg)) {
> +               err = PTR_ERR(cfg);
> +               pr_err("%04x:%02x-%02x error %d mapping CAM\n", seg,
> +                       bus_start, bus_end, err);
> +               goto err_out;
> +       }

You seem to have moved all the config space mapping to this
point. Intel seems to do the mapping when they read MCFG for the
entries, and I had followed that model, and that avoids having another
array/list to save the values.

> +       cfg->domain = seg;
> +       ri->cfg = cfg;
> +err_out:
> +       mutex_unlock(&pci_mcfg_lock);
> +       return err;
> +}
> +
> +/* release_info: free resrouces allocated by init_info */
> +static void pci_acpi_generic_release_info(struct acpi_pci_root_info *ci)
> +{
> +       struct acpi_pci_generic_root_info *ri;
> +
> +       ri = container_of(ci, struct acpi_pci_generic_root_info, common);
> +       pci_generic_ecam_free(ri->cfg);
> +       kfree(ri);
> +}
> +
> +static struct acpi_pci_root_ops acpi_pci_root_ops = {
> +       .release_info = pci_acpi_generic_release_info,
> +};
> +
> +/* Interface called from ACPI code to setup PCI host controller */
> +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> +{
> +       int node = acpi_get_node(root->device->handle);
> +       struct acpi_pci_generic_root_info *ri;
> +       struct pci_bus *bus, *child;
> +       int err;
> +
> +       ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node);
> +       if (!ri)
> +               return NULL;
> +
> +       err = pci_acpi_setup_ecam_mapping(root, ri);
> +       if (err)
> +               return NULL;
> +
> +       acpi_pci_root_ops.pci_ops = &ri->cfg->ops->pci_ops;
> +       bus = acpi_pci_root_create(root, &acpi_pci_root_ops, &ri->common,
> +                                  ri->cfg);
> +       if (!bus)
> +               return NULL;
> +
> +       pci_bus_size_bridges(bus);
> +       pci_bus_assign_resources(bus);
> +
> +       list_for_each_entry(child, &bus->children, node)
> +               pcie_bus_configure_settings(child);
> +
> +       return bus;
> +}
> +
> +/* handle MCFG table entries */
> +static __init int pci_mcfg_parse(struct acpi_table_header *header)
> +{
> +       struct acpi_table_mcfg *mcfg;
> +       struct acpi_mcfg_allocation *mptr;
> +       struct mcfg_entry *e, *arr;
> +       int i, n;
> +
> +       if (!header)
> +               return -EINVAL;
> +
> +       mcfg = (struct acpi_table_mcfg *)header;
> +       mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
> +       n = (header->length - sizeof(*mcfg)) / sizeof(*mptr);
> +       if (n <= 0 || n > 255) {
> +               pr_err(PREFIX " MCFG has incorrect entries (%d).\n", n);
> +               return -EINVAL;
> +       }
> +
> +       arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
> +       if (!arr)
> +               return -ENOMEM;

Here you already have an array which is also connected as a linked
list which is unnecessary.

> +       for (i = 0, e = arr; i < n; i++, mptr++, e++) {
> +               e->segment = mptr->pci_segment;
> +               e->addr =  mptr->address;
> +               e->bus_start = mptr->start_bus_number;
> +               e->bus_end = mptr->end_bus_number;
> +               list_add(&e->list, &pci_mcfg_list);
> +               pr_info(PREFIX
> +                       "MCFG entry for domain %04x [bus %02x-%02x] (base %pa)\n",
> +                       e->segment, e->bus_start, e->bus_end, &e->addr);
> +       }
> +
> +       return 0;
> +}
> +
> +/* Interface called by ACPI - parse and save MCFG table */
> +void __init pci_mcfg_init(void)
> +{
> +       int err = acpi_table_parse(ACPI_SIG_MCFG, pci_mcfg_parse);
> +       if (err)
> +               pr_err(PREFIX "Failed to parse MCFG (%d)\n", err);
> +       else if (list_empty(&pci_mcfg_list))
> +               pr_info(PREFIX "No valid entries in MCFG table.\n");
> +       else {
> +               struct mcfg_entry *e;
> +               int i = 0;
> +               list_for_each_entry(e, &pci_mcfg_list, list)
> +                       i++;
> +               pr_info(PREFIX "MCFG table loaded, %d entries\n", i);
> +       }
> +}
> +
> +/* Raw operations, works only for MCFG entries with an associated bus */
> +int raw_pci_read(unsigned int domain, unsigned int busn, unsigned int devfn,
> +                int reg, int len, u32 *val)
> +{
> +       struct pci_bus *bus = pci_find_bus(domain, busn);
> +
> +       if (!bus)
> +               return PCIBIOS_DEVICE_NOT_FOUND;
> +       return bus->ops->read(bus, devfn, reg, len, val);
> +}
> +
> +int raw_pci_write(unsigned int domain, unsigned int busn, unsigned int devfn,
> +                 int reg, int len, u32 val)
> +{
> +       struct pci_bus *bus = pci_find_bus(domain, busn);
> +
> +       if (!bus)
> +               return PCIBIOS_DEVICE_NOT_FOUND;
> +       return bus->ops->write(bus, devfn, reg, len, val);
> +}
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index df1f33d..c0422ea 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1729,6 +1729,12 @@ static inline void pci_mmcfg_early_init(void) { }
>  static inline void pci_mmcfg_late_init(void) { }
>  #endif
>
> +#ifdef CONFIG_ACPI_PCI_HOST_GENERIC
> +void __init pci_mcfg_init(void);
> +#else
> +static inline void pci_mcfg_init(void) { return; }
> +#endif

You can still use the function pci_mmcfg_late_init() if
PCI_MMCONFIG or ACPI_PCI_HOST_GENERIC is defined

> +
>  int pci_ext_cfg_avail(void);
>
>  void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar);


JC.