On Tue, May 10, 2016 at 8:49 PM, Tomasz Nowicki <tn@xxxxxxxxxxxx> wrote:
> This patch is going to implement generic PCI host controller for
> ACPI world, similar to what pci-host-generic.c driver does for DT world.
>
> All such drivers, which we have seen so far, were implemented within
> arch/ directory since they had some arch assumptions (x86 and ia64).
> However, they all are doing similar thing, so it makes sense to find
> some common code and abstract it into the generic driver.
>
> In order to handle PCI config space regions properly, we define new
> MCFG interface which does sanity checks on MCFG table and keeps its
> root pointer. User is able to lookup MCFG regions based on that root
> pointer and specified domain:bus_start:bus_end touple. We are using
> pci_mmcfg_late_init old prototype to avoid another function name.
>
> The implementation of pci_acpi_scan_root() looks up the MCFG entries
> and sets up a new mapping (regions are not mapped until host controller ask
> for it). Generic PCI functions are used for accessing config space.
> Driver selects PCI_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.
Looking thru the new code, I see a few issues, please see below
> Signed-off-by: Tomasz Nowicki <tn@xxxxxxxxxxxx>
> Signed-off-by: Jayachandran C <jchandra@xxxxxxxxxxxx>
> ---
> drivers/acpi/Kconfig | 8 +++
> drivers/acpi/Makefile | 1 +
> drivers/acpi/pci_mcfg.c | 97 ++++++++++++++++++++++++++
> drivers/acpi/pci_root_generic.c | 149 ++++++++++++++++++++++++++++++++++++++++
> drivers/pci/ecam.h | 5 ++
> include/linux/pci-acpi.h | 5 ++
> include/linux/pci.h | 5 +-
> 7 files changed, 269 insertions(+), 1 deletion(-)
> create mode 100644 drivers/acpi/pci_mcfg.c
> create mode 100644 drivers/acpi/pci_root_generic.c
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 183ffa3..44afc76 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_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..627a2b7 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_root_generic.o pci_mcfg.o
> acpi-y += acpi_lpss.o acpi_apd.o
> acpi-y += acpi_platform.o
> acpi-y += acpi_pnp.o
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> new file mode 100644
> index 0000000..373d079
> --- /dev/null
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -0,0 +1,97 @@
> +/*
> + * Copyright (C) 2016 Broadcom
> + * Author: Jayachandran C <jchandra@xxxxxxxxxxxx>
> + * Copyright (C) 2016 Semihalf
> + * Author: Tomasz Nowicki <tn@xxxxxxxxxxxx>
> + *
> + * 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>
> +
> +#define PREFIX "ACPI: "
> +
> +/* Root pointer to the mapped MCFG table */
> +static struct acpi_table_mcfg *mcfg_table;
> +
> +#define MCFG_ENTRIES(mcfg_ptr) (((mcfg_ptr)->header.length - \
> + sizeof(struct acpi_table_mcfg)) / \
> + sizeof(struct acpi_mcfg_allocation))
It would be better if you used static inline function here.
> +static phys_addr_t pci_mcfg_lookup_static(u16 seg, u8 bus_start, u8 bus_end)
> +{
> + struct acpi_mcfg_allocation *mptr;
> + int i;
> +
> + if (!mcfg_table) {
> + pr_err(PREFIX "MCFG table not available, lookup failed\n");
> + return -ENXIO;
> + }
> +
> + mptr = (struct acpi_mcfg_allocation *) &mcfg_table[1];
> +
> + /*
> + * We expect exact match, unless MCFG entry end bus covers more than
> + * specified by caller.
> + */
> + for (i = 0; i < MCFG_ENTRIES(mcfg_table); i++, mptr++) {
> + if (mptr->pci_segment == seg &&
> + mptr->start_bus_number == bus_start &&
> + mptr->end_bus_number >= bus_end) {
> + return mptr->address;
> + }
> + }
There is an issue here, the bus range is obtained if different
ways. If the _CRS has it, this should be fine. But if it is _BBN-0xff
or the default 0-0xff, then I would think taking the MCFG entry end
would be better. This would require updating the bus resource end.
Also (trivial), the braces are not needed.
> + return -ENXIO;
> +}
> +
> +phys_addr_t pci_mcfg_lookup(struct acpi_device *device, u16 seg,
> + struct resource *bus_res)
> +{
> + phys_addr_t addr;
> +
> + addr = acpi_pci_root_get_mcfg_addr(device->handle);
> + if (addr)
> + return addr;
When you have address from _CBA, you are assuming that the
bus range is also set correctly (from _CRS or as _BBN-0xff). Is
this assumption correct?
(this was discussed earlier) you are doing the _CBA call again,
I think cleaning up the _CBA, _BBN, _CRS and MCFG based
ECAM area resources and defaults should be a different patch,
which would need changes to pci_root.c. We should use
root->mcfg_addr in this patchset.
> +
> + return pci_mcfg_lookup_static(seg, bus_res->start, bus_res->end);
There is no need to have a separate function for this, based on by
above comment, you might need to change bus_res, so having it here
would be better.
> +}
> +
> +static __init int pci_mcfg_parse(struct acpi_table_header *header)
> +{
> + struct acpi_table_mcfg *mcfg;
> + int n;
> +
> + if (!header)
> + return -EINVAL;
This is not needed, the handler is not called if header is NULL
> +
> + mcfg = (struct acpi_table_mcfg *)header;
> + n = MCFG_ENTRIES(mcfg);
> + if (n <= 0 || n > 255) {
> + pr_err(PREFIX "MCFG has incorrect entries (%d).\n", n);
> + return -EINVAL;
> + }
>
> + mcfg_table = mcfg;
Saving a reference of ACPI mapping seems dangerous, acpi_parse_table
calles early_acpi_os_unmap_memory() after calling handler, which does
not do anything since acpi_gbl_permanent_mmap is set.
I would suggest copying the entries.
> + pr_info(PREFIX "MCFG table loaded, %d entries detected\n", n);
> + return 0;
> +}
> +
> +/* Interface called by ACPI - parse and save MCFG table */
> +void __init pci_mmcfg_late_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);
> +}
JC.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
