| Message ID | 1460740008-19489-10-git-send-email-tn@semihalf.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Fri, Apr 15, 2016 at 10:36 PM, Tomasz Nowicki <tn@semihalf.com> 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@semihalf.com> > Signed-off-by: Jayachandran C <jchandra@broadcom.com> > --- > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20.04.2016 21:12, Jayachandran C wrote: > On Fri, Apr 15, 2016 at 10:36 PM, Tomasz Nowicki <tn@semihalf.com> 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. We all agree this too important for everybody to delay this series. So main motivation is to keep all discussion&patches within one unified series. I would like to finally find direction we need to go. Stating another discussion based on my previous patch set v5 confused people, they do no know who is driving this. Again, lets cooperate to move it forward within one patch set. I agree with you we need maintainers to join this discussion. > > 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. The next version will not have quirk handling part. Regarding your comments, please see below. > >> Signed-off-by: Tomasz Nowicki <tn@semihalf.com> >> Signed-off-by: Jayachandran C <jchandra@broadcom.com> >> --- >> 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. I rather forgot to add copyright here, I will fix it. > >> + * 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 PCI driver supports hot plug/removal, I want to avoid races using this lock. I decided to use list because I do ECAM mapping on demand. See below for more details. > >> +/* 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 put it here as per Bjorn's request, see: https://lkml.org/lkml/2016/3/4/946 If this is not valid any more, I can easily remove it. > > 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. See: https://lkml.org/lkml/2016/3/3/921 I agree with Bjorn here, we should map it whenever we need this instead of mapping all MCFG entries just in case. Also, I see other advantages: 1. We can always use valid "dev" for pci_generic_ecam_create call, what you can't do when you use pci_generic_ecam_create during MCFG parsing. 2. No need for special handling for entries coming from _CBA, the path is the same for MCFG and _CBA. We do not have to remember that only _CBA related entries have to be unmapped. > >> + 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. The array is to avoid complicated error handling in case the allocation for some of element would failed. I can rework this to allocate each element separately or to use array but the code is simpler now. As explained above (on demand mapping), I would like to keep list/array with MCFG entries (I preferred list). > >> + 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 > OK -#ifdef CONFIG_PCI_MMCONFIG +#if defined(CONFIG_PCI_MMCONFIG) || defined(ACPI_PCI_HOST_GENERIC) void __init pci_mmcfg_early_init(void); void __init pci_mmcfg_late_init(void); #else static inline void pci_mmcfg_early_init(void) { } static inline void pci_mmcfg_late_init(void) { } #endif is that what you mean? Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 21, 2016 at 2:36 PM, Tomasz Nowicki <tn@semihalf.com> wrote: > On 20.04.2016 21:12, Jayachandran C wrote: >> >> On Fri, Apr 15, 2016 at 10:36 PM, Tomasz Nowicki <tn@semihalf.com> 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. > > > We all agree this too important for everybody to delay this series. So main > motivation is to keep all discussion&patches within one unified series. I > would like to finally find direction we need to go. Stating another > discussion based on my previous patch set v5 confused people, they do no > know who is driving this. Again, lets cooperate to move it forward within > one patch set. > > I agree with you we need maintainers to join this discussion. > >> >> 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. > > > The next version will not have quirk handling part. Regarding your comments, > please see below. > > >> >>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com> >>> Signed-off-by: Jayachandran C <jchandra@broadcom.com> >>> --- >>> 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. > > > I rather forgot to add copyright here, I will fix it. > > >> >>> + * 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 PCI driver supports hot plug/removal, I want to avoid races using this > lock. I decided to use list because I do ECAM mapping on demand. See below > for more details. Yes, there is hotplug. but there is no change to the saved MCFG entries after you create the array (or list). And, the ECAM mapping is on demand but it also just does a lookup and does not modify the array/list, so the locking is not needed. There may be a locking issue for hotplug in raw_pci_read/write between find_bus and bus->ops->read/write, but this does not solve that. My expectation is that since both are ACPI originated this may not be an issue. >> >>> +/* 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 put it here as per Bjorn's request, see: > https://lkml.org/lkml/2016/3/4/946 > > If this is not valid any more, I can easily remove it. There are multiple ways in which acpi_pci_root_add() tries get the bus range and mcfg address for a root bus. I think Bjorn's suggestion was to add looking up the MCFG there too, and I think it would be better if it was done in a separate patchset. >> >> 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. > > > See: > https://lkml.org/lkml/2016/3/3/921 > > I agree with Bjorn here, we should map it whenever we need this instead of > mapping all MCFG entries just in case. Also, I see other advantages: > 1. We can always use valid "dev" for pci_generic_ecam_create call, what you > can't do when you use pci_generic_ecam_create during MCFG parsing. > 2. No need for special handling for entries coming from _CBA, the path is > the same for MCFG and _CBA. We do not have to remember that only _CBA > related entries have to be unmapped. > > >> >>> + 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. > > > The array is to avoid complicated error handling in case the allocation for > some of element would failed. I can rework this to allocate each element > separately or to use array but the code is simpler now. As explained above > (on demand mapping), I would like to keep list/array with MCFG entries (I > preferred list). > > >> >>> + 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 >> > > OK > > -#ifdef CONFIG_PCI_MMCONFIG > +#if defined(CONFIG_PCI_MMCONFIG) || defined(ACPI_PCI_HOST_GENERIC) > void __init pci_mmcfg_early_init(void); > void __init pci_mmcfg_late_init(void); > #else > static inline void pci_mmcfg_early_init(void) { } > static inline void pci_mmcfg_late_init(void) { } > #endif > > is that what you mean? Yes - this should be sufficient instead of a new function... JC. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/21/2016 05:06 AM, Tomasz Nowicki wrote: > On 20.04.2016 21:12, Jayachandran C wrote: >> On Fri, Apr 15, 2016 at 10:36 PM, Tomasz Nowicki <tn@semihalf.com> wrote: >>> 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. > > We all agree this too important for everybody to delay this series. So > main motivation is to keep all discussion&patches within one unified > series. I would like to finally find direction we need to go. Stating > another discussion based on my previous patch set v5 confused people, > they do no know who is driving this. Again, lets cooperate to move it > forward within one patch set. We need one person in the driver's seat here for this patch series. I believe the intention is that this is Tomasz, with others cooperating and assisting. The previous alternative patch series did serve to cause confusion, and worse, they made it look like the ARM vendors can't work together. That ends. Right now. I've raised this individually with each of you (and with all of the other vendors), as well as inside Linaro. There will be one person driving this, and everyone else will help. > I agree with you we need maintainers to join this discussion. Please. I really want to lend support to the sense of urgency of getting something merged that provides the basic ACPI based ECAM functionality on ARMv8 systems. Until that happens, there are vendors who are going to have to delay various other activities around ARM server until it is done (because of the lack of an upstream patch is a critical blocker - this isn't embedded, and we don't work that way). This is incredibly critical and central to the successful adoption of larger ARMv8 servers over the next year or so, all of which will rely upon PCIe, using ACPI. We therefore need all hands on deck, and all vendors getting overwhelmingly excited about making this patch series a success. As an aside, to help the engineering orgs within these vendors understand how much I need motion on this thread, I'm buzzing in their ears multiple times per week specifically on engagement on this thread. We need this done. If Bjorn or anyone else needs something, tell us. Jon.
On 04/22/2016 10:40 AM, Jon Masters wrote: > On 04/21/2016 05:06 AM, Tomasz Nowicki wrote: >> On 20.04.2016 21:12, Jayachandran C wrote: >>> On Fri, Apr 15, 2016 at 10:36 PM, Tomasz Nowicki <tn@semihalf.com> wrote: > >>>> 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. >> >> We all agree this too important for everybody to delay this series. So >> main motivation is to keep all discussion&patches within one unified >> series. I would like to finally find direction we need to go. Stating >> another discussion based on my previous patch set v5 confused people, >> they do no know who is driving this. Again, lets cooperate to move it >> forward within one patch set. > > We need one person in the driver's seat here for this patch series. I > believe the intention is that this is Tomasz, with others cooperating > and assisting. The previous alternative patch series did serve to cause > confusion, and worse, they made it look like the ARM vendors can't work > together. That ends. Right now. I've raised this individually with each > of you (and with all of the other vendors), as well as inside Linaro. > There will be one person driving this, and everyone else will help. As a quick update, since yesterday I have confirmed that several different microarchitecture implementations (different PCIe) have tested and validated this patch series. Those minimally include: * Cavium Networks ThunderX * Qualcomm Technologies Inc QDF2XXX * AMD A1100 ("Seattle") Another is working on testing over the weekend. Still waiting for an ARM tested-by on Juno I think. I will personally be testing this and future releases on all of the above mentioned hw. Jon.
Hey, I really kind of like this. I think this might work out well. On Fri, Apr 15, 2016 at 07:06:44PM +0200, Tomasz Nowicki 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) > > Signed-off-by: Tomasz Nowicki <tn@semihalf.com> > Signed-off-by: Jayachandran C <jchandra@broadcom.com> > --- > 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 Maybe "pci_root_generic.c" so it shows up next to and is obviously related to pci_root.c? > 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(); > 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 @@ > +/* > + * 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); > + > +/* 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; > +} Can you put the MCFG parsing, caching, and searching in a different file, e.g., drivers/acpi/pci_mcfg.c? > +/* > + * 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); What does this lock protect? The pci_mcfg_list should already be initialized by the time we get there, and it should be immutable for the life of the system. In fact, I would prefer if we could just search the static table itself whenever we need it rather than caching it in our own list. But I don't think we can easily do that because acpi_table_parse() is __init. > + e = pci_mcfg_lookup(seg, bus_start); I would argue that we should check for _CBA first, and fall back to MCFG if _CBA doesn't exist. > + if (!e) { > + addr = acpi_pci_root_get_mcfg_addr(root->device->handle); IMO, acpi_pci_root_get_mcfg_addr() is misnamed. It should be acpi_pci_config_base_addr() or similar. It definitely is not related to MCFG. Not your fault, obviously. > + 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; > + } I really don't think you need a lock around this, so you can factor out the address lookup into something like: addr = acpi_pci_config_base_addr(...); if (addr) return addr; return acpi_pci_mcfg_lookup(seg, busn_res); You can check inside acpi_pci_mcfg_lookup() to make sure the entry you find covers the entire [busn_res.start-busn_res.end] range and return failure if it doesn't. At this point, I'm not sure it's worth it to truncate the host bridge bus range to match something we find in MCFG. If the MCFG entry covers *more* than the host bridge range from _CRS, that's fine. In any case, we have to be careful with the start address, because the MCFG start address is always based on bus 0, but I think pci_generic_ecam_create() expects the start address based on the bus_start you pass to it. > + > + 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; > + } > + > + 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; > + > + 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); Ah, this is information similar to what I suggested we might print in pci_generic_ecam_create(). Could go either way, but personally I think I'd put it in pci_generic_ecam_create() instead, because then we only print the info we're actually using. And I think it'd be nice to have the actual MMIO resource ("[mem 0x...-0x...]") instead of just the base. > + } > + > + 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 */ Can you elaborate a little on the connection with MCFG? I don't quite see how they're related. Obviously the device has to be on a bus we've already enumerated and assigned bus->ops for, but it seems like we don't really know or care what bus->ops actually is. Given that these are in this file, acpi_pci_root_ops is the only possibility, since that's what we pass to acpi_pci_root_create(), but it doesn't seem worth mentioning specifically in a comment. > +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 > + > int pci_ext_cfg_avail(void); > > void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar); > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 28, 2016 at 04:48:00PM -0500, Bjorn Helgaas wrote: [...] > > +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); > > What does this lock protect? The pci_mcfg_list should already be > initialized by the time we get there, and it should be immutable for > the life of the system. In fact, I would prefer if we could just > search the static table itself whenever we need it rather than caching > it in our own list. But I don't think we can easily do that because > acpi_table_parse() is __init. > > > + e = pci_mcfg_lookup(seg, bus_start); > > I would argue that we should check for _CBA first, and fall back to > MCFG if _CBA doesn't exist. > > > + if (!e) { > > + addr = acpi_pci_root_get_mcfg_addr(root->device->handle); > > IMO, acpi_pci_root_get_mcfg_addr() is misnamed. It should be > acpi_pci_config_base_addr() or similar. It definitely is not related > to MCFG. Not your fault, obviously. > > > + 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; > > + } > > I really don't think you need a lock around this, so you can factor > out the address lookup into something like: > > addr = acpi_pci_config_base_addr(...); > if (addr) > return addr; > > return acpi_pci_mcfg_lookup(seg, busn_res); > > You can check inside acpi_pci_mcfg_lookup() to make sure the entry you > find covers the entire [busn_res.start-busn_res.end] range and return > failure if it doesn't. At this point, I'm not sure it's worth it to > truncate the host bridge bus range to match something we find in MCFG. > > If the MCFG entry covers *more* than the host bridge range from _CRS, > that's fine. In any case, we have to be careful with the start address, > because the MCFG start address is always based on bus 0, but I think > pci_generic_ecam_create() expects the start address based on the > bus_start you pass to it. Yes, I spotted this too, it is unfortunate but DT and MCFG handle the ECAM regions differently. In DT the reg property is relative to bus_start - ie reg MMIO region maps config space starting at the first bus in bus-range: Documentation/devicetree/bindings/pci/host-generic-pci.txt in ACPI(MCFG) as you said it is always relative to bus 0, it is unfortunate but the address to be mapped should be computed differently in the ECAM layer. Lorenzo -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 29, 2016 at 2:07 PM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Thu, Apr 28, 2016 at 04:48:00PM -0500, Bjorn Helgaas wrote: > > [...] > >> > +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); >> >> What does this lock protect? The pci_mcfg_list should already be >> initialized by the time we get there, and it should be immutable for >> the life of the system. In fact, I would prefer if we could just >> search the static table itself whenever we need it rather than caching >> it in our own list. But I don't think we can easily do that because >> acpi_table_parse() is __init. >> >> > + e = pci_mcfg_lookup(seg, bus_start); >> >> I would argue that we should check for _CBA first, and fall back to >> MCFG if _CBA doesn't exist. >> >> > + if (!e) { >> > + addr = acpi_pci_root_get_mcfg_addr(root->device->handle); >> >> IMO, acpi_pci_root_get_mcfg_addr() is misnamed. It should be >> acpi_pci_config_base_addr() or similar. It definitely is not related >> to MCFG. Not your fault, obviously. >> >> > + 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; >> > + } >> >> I really don't think you need a lock around this, so you can factor >> out the address lookup into something like: >> >> addr = acpi_pci_config_base_addr(...); >> if (addr) >> return addr; >> >> return acpi_pci_mcfg_lookup(seg, busn_res); >> >> You can check inside acpi_pci_mcfg_lookup() to make sure the entry you >> find covers the entire [busn_res.start-busn_res.end] range and return >> failure if it doesn't. At this point, I'm not sure it's worth it to >> truncate the host bridge bus range to match something we find in MCFG. >> >> If the MCFG entry covers *more* than the host bridge range from _CRS, >> that's fine. In any case, we have to be careful with the start address, >> because the MCFG start address is always based on bus 0, but I think >> pci_generic_ecam_create() expects the start address based on the >> bus_start you pass to it. > > Yes, I spotted this too, it is unfortunate but DT and MCFG handle > the ECAM regions differently. In DT the reg property is relative > to bus_start - ie reg MMIO region maps config space starting at > the first bus in bus-range: > > Documentation/devicetree/bindings/pci/host-generic-pci.txt > > in ACPI(MCFG) as you said it is always relative to bus 0, it is > unfortunate but the address to be mapped should be computed > differently in the ECAM layer. Can't this be handled by fixing up the address before passing to pci_generic_ecam_create? JC. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/28/2016 11:48 PM, Bjorn Helgaas wrote: > Hey, I really kind of like this. I think this might work out well. > > On Fri, Apr 15, 2016 at 07:06:44PM +0200, Tomasz Nowicki 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) >> >> Signed-off-by: Tomasz Nowicki <tn@semihalf.com> >> Signed-off-by: Jayachandran C <jchandra@broadcom.com> >> --- >> 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 > Maybe "pci_root_generic.c" so it shows up next to and is obviously > related to pci_root.c? Yes, this is good idea. > >> 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(); >> 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 @@ >> +/* >> + * 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); >> + >> +/* 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; >> +} > Can you put the MCFG parsing, caching, and searching in a different > file, e.g., drivers/acpi/pci_mcfg.c? I will. > >> +/* >> + * 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); > What does this lock protect? The pci_mcfg_list should already be > initialized by the time we get there, and it should be immutable for > the life of the system. Right, lock is useless. > In fact, I would prefer if we could just > search the static table itself whenever we need it rather than caching > it in our own list. But I don't think we can easily do that because > acpi_table_parse() is __init. It is doable. We can implement our MCFG __init parsing handle to do necessary sanity checks and then store MCFG table root pointer in pci_mcfg.c. Then lookup call would use that pointer to traverse available entries on demand. > >> + e = pci_mcfg_lookup(seg, bus_start); > I would argue that we should check for _CBA first, and fall back to > MCFG if _CBA doesn't exist. Agree. > >> + if (!e) { >> + addr = acpi_pci_root_get_mcfg_addr(root->device->handle); > IMO, acpi_pci_root_get_mcfg_addr() is misnamed. It should be > acpi_pci_config_base_addr() or similar. It definitely is not related > to MCFG. Not your fault, obviously. > >> + 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; >> + } > I really don't think you need a lock around this, so you can factor > out the address lookup into something like: > > addr = acpi_pci_config_base_addr(...); > if (addr) > return addr; > > return acpi_pci_mcfg_lookup(seg, busn_res); > > You can check inside acpi_pci_mcfg_lookup() to make sure the entry you > find covers the entire [busn_res.start-busn_res.end] range and return > failure if it doesn't. At this point, I'm not sure it's worth it to > truncate the host bridge bus range to match something we find in MCFG. > > If the MCFG entry covers *more* than the host bridge range from _CRS, > that's fine. Makes sense for me. > In any case, we have to be careful with the start address, > because the MCFG start address is always based on bus 0, but I think > pci_generic_ecam_create() expects the start address based on the > bus_start you pass to it. We will have a look how to fix this. > >> + >> + 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; >> + } >> + >> + 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; >> + >> + 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); > Ah, this is information similar to what I suggested we might print in > pci_generic_ecam_create(). Could go either way, but personally I > think I'd put it in pci_generic_ecam_create() instead, because then we > only print the info we're actually using. And I think it'd be nice to > have the actual MMIO resource ("[mem 0x...-0x...]") instead of just > the base. Giving above that I will drop MCFG entries cache, this will be printed in pci_generic_ecam_create, as you suggested. > >> + } >> + >> + 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 */ > Can you elaborate a little on the connection with MCFG? I don't quite > see how they're related. Obviously the device has to be on a bus > we've already enumerated and assigned bus->ops for, but it seems like > we don't really know or care what bus->ops actually is. Given that > these are in this file, acpi_pci_root_ops is the only possibility, > since that's what we pass to acpi_pci_root_create(), but it doesn't > seem worth mentioning specifically in a comment. Yes, you are right. I will drop this comment. > >> +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 >> + >> int pci_ext_cfg_avail(void); >> >> void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar); >> -- >> 1.9.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-pci" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html Thanks, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/29/2016 07:35 PM, Jayachandran C wrote: > On Fri, Apr 29, 2016 at 2:07 PM, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: >> On Thu, Apr 28, 2016 at 04:48:00PM -0500, Bjorn Helgaas wrote: >> >> [...] >> >>>> +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); >>> What does this lock protect? The pci_mcfg_list should already be >>> initialized by the time we get there, and it should be immutable for >>> the life of the system. In fact, I would prefer if we could just >>> search the static table itself whenever we need it rather than caching >>> it in our own list. But I don't think we can easily do that because >>> acpi_table_parse() is __init. >>> >>>> + e = pci_mcfg_lookup(seg, bus_start); >>> I would argue that we should check for _CBA first, and fall back to >>> MCFG if _CBA doesn't exist. >>> >>>> + if (!e) { >>>> + addr = acpi_pci_root_get_mcfg_addr(root->device->handle); >>> IMO, acpi_pci_root_get_mcfg_addr() is misnamed. It should be >>> acpi_pci_config_base_addr() or similar. It definitely is not related >>> to MCFG. Not your fault, obviously. >>> >>>> + 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; >>>> + } >>> I really don't think you need a lock around this, so you can factor >>> out the address lookup into something like: >>> >>> addr = acpi_pci_config_base_addr(...); >>> if (addr) >>> return addr; >>> >>> return acpi_pci_mcfg_lookup(seg, busn_res); >>> >>> You can check inside acpi_pci_mcfg_lookup() to make sure the entry you >>> find covers the entire [busn_res.start-busn_res.end] range and return >>> failure if it doesn't. At this point, I'm not sure it's worth it to >>> truncate the host bridge bus range to match something we find in MCFG. >>> >>> If the MCFG entry covers *more* than the host bridge range from _CRS, >>> that's fine. In any case, we have to be careful with the start address, >>> because the MCFG start address is always based on bus 0, but I think >>> pci_generic_ecam_create() expects the start address based on the >>> bus_start you pass to it. >> Yes, I spotted this too, it is unfortunate but DT and MCFG handle >> the ECAM regions differently. In DT the reg property is relative >> to bus_start - ie reg MMIO region maps config space starting at >> the first bus in bus-range: >> >> Documentation/devicetree/bindings/pci/host-generic-pci.txt >> >> in ACPI(MCFG) as you said it is always relative to bus 0, it is >> unfortunate but the address to be mapped should be computed >> differently in the ECAM layer. > Can't this be handled by fixing up the address before passing to > pci_generic_ecam_create? > I agree, this should work, IMO. Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 29, 2016 at 11:05:34PM +0530, Jayachandran C wrote: > On Fri, Apr 29, 2016 at 2:07 PM, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > On Thu, Apr 28, 2016 at 04:48:00PM -0500, Bjorn Helgaas wrote: > > > > [...] > > > >> > +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); > >> > >> What does this lock protect? The pci_mcfg_list should already be > >> initialized by the time we get there, and it should be immutable for > >> the life of the system. In fact, I would prefer if we could just > >> search the static table itself whenever we need it rather than caching > >> it in our own list. But I don't think we can easily do that because > >> acpi_table_parse() is __init. > >> > >> > + e = pci_mcfg_lookup(seg, bus_start); > >> > >> I would argue that we should check for _CBA first, and fall back to > >> MCFG if _CBA doesn't exist. > >> > >> > + if (!e) { > >> > + addr = acpi_pci_root_get_mcfg_addr(root->device->handle); > >> > >> IMO, acpi_pci_root_get_mcfg_addr() is misnamed. It should be > >> acpi_pci_config_base_addr() or similar. It definitely is not related > >> to MCFG. Not your fault, obviously. > >> > >> > + 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; > >> > + } > >> > >> I really don't think you need a lock around this, so you can factor > >> out the address lookup into something like: > >> > >> addr = acpi_pci_config_base_addr(...); > >> if (addr) > >> return addr; > >> > >> return acpi_pci_mcfg_lookup(seg, busn_res); > >> > >> You can check inside acpi_pci_mcfg_lookup() to make sure the entry you > >> find covers the entire [busn_res.start-busn_res.end] range and return > >> failure if it doesn't. At this point, I'm not sure it's worth it to > >> truncate the host bridge bus range to match something we find in MCFG. > >> > >> If the MCFG entry covers *more* than the host bridge range from _CRS, > >> that's fine. In any case, we have to be careful with the start address, > >> because the MCFG start address is always based on bus 0, but I think > >> pci_generic_ecam_create() expects the start address based on the > >> bus_start you pass to it. > > > > Yes, I spotted this too, it is unfortunate but DT and MCFG handle > > the ECAM regions differently. In DT the reg property is relative > > to bus_start - ie reg MMIO region maps config space starting at > > the first bus in bus-range: > > > > Documentation/devicetree/bindings/pci/host-generic-pci.txt > > > > in ACPI(MCFG) as you said it is always relative to bus 0, it is > > unfortunate but the address to be mapped should be computed > > differently in the ECAM layer. > > Can't this be handled by fixing up the address before passing to > pci_generic_ecam_create? Yes it can, you just need to apply the bus shift, given that we know it is ECAM anyway you can even add a macro in the ecam generic header to compute it, anyway that's a minor detail, we just should not forget to fix it. Lorenzo -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(); 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 @@ +/* + * 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); + +/* 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); + 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; + } + + 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; + + 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 + int pci_ext_cfg_avail(void); void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar);