[PATCH V7 00/11] Support for generic ACPI based PCI host controller

From: Jayachandran C <***@broadcom.com>

Use functions provided by drivers/pci/ecam.h for mapping the config
space in drivers/pci/host/pci-host-common.c, and update its users to
use 'struct pci_config_window' and 'struct pci_ecam_ops'

The changes are mostly to use 'struct pci_config_window' in place of
'struct gen_pci'. Some of the fields of gen_pci were only used
temporarily and can be eliminated by using local variables or function
arguments, these are not carried over to struct pci_config_window.

pci-thunder-ecam.c and pci-thunder-pem.c are the only users of the
pci_host_common_probe function and the gen_pci structure, these have
been updated to use the new API as well.

The patch does not introduce any functional changes other than a very
minor one: with the new code, on 64-bit platforms, we do just a single
ioremap for the whole config space.

Signed-off-by: Jayachandran C <***@broadcom.com>
---
drivers/pci/ecam.h | 5 ++
drivers/pci/host/Kconfig | 1 +
drivers/pci/host/pci-host-common.c | 114 +++++++++++++++---------------------
drivers/pci/host/pci-host-common.h | 47 ---------------
drivers/pci/host/pci-host-generic.c | 52 ++++------------
drivers/pci/host/pci-thunder-ecam.c | 39 +++---------
drivers/pci/host/pci-thunder-pem.c | 92 ++++++++++++++---------------
7 files changed, 113 insertions(+), 237 deletions(-)
delete mode 100644 drivers/pci/host/pci-host-common.h

diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h
index 57de00d..1ad2176 100644
--- a/drivers/pci/ecam.h
+++ b/drivers/pci/ecam.h
@@ -59,4 +59,9 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
/* default ECAM ops, bus shift 20, generic read and write */
extern struct pci_ecam_ops pci_generic_ecam_ops;

+#ifdef CONFIG_PCI_HOST_GENERIC
+/* for DT based pci controllers that support ECAM */
+int pci_host_common_probe(struct platform_device *pdev,
+ struct pci_ecam_ops *ops);
+#endif
#endif
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 7a0780d..963300d 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -77,6 +77,7 @@ config PCI_RCAR_GEN2_PCIE

config PCI_HOST_COMMON
bool
+ select PCI_ECAM

config PCI_HOST_GENERIC
bool "Generic PCI host controller"
diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c
index e9f850f..8cba7ab 100644
--- a/drivers/pci/host/pci-host-common.c
+++ b/drivers/pci/host/pci-host-common.c
@@ -22,27 +22,21 @@
#include <linux/of_pci.h>
#include <linux/platform_device.h>

-#include "pci-host-common.h"
+#include "../ecam.h"

-static void gen_pci_release_of_pci_ranges(struct gen_pci *pci)
-{
- pci_free_resource_list(&pci->resources);
-}
-
-static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci)
+static int gen_pci_parse_request_of_pci_ranges(struct device *dev,
+ struct list_head *resources, struct resource **bus_range)
{
int err, res_valid = 0;
- struct device *dev = pci->host.dev.parent;
struct device_node *np = dev->of_node;
resource_size_t iobase;
struct resource_entry *win;

- err = of_pci_get_host_bridge_resources(np, 0, 0xff, &pci->resources,
- &iobase);
+ err = of_pci_get_host_bridge_resources(np, 0, 0xff, resources, &iobase);
if (err)
return err;

- resource_list_for_each_entry(win, &pci->resources) {
+ resource_list_for_each_entry(win, resources) {
struct resource *parent, *res = win->res;

switch (resource_type(res)) {
@@ -60,7 +54,7 @@ static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci)
res_valid |= !(res->flags & IORESOURCE_PREFETCH);
break;
case IORESOURCE_BUS:
- pci->cfg.bus_range = res;
+ *bus_range = res;
default:
continue;
}
@@ -79,65 +73,60 @@ static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci)
return 0;

out_release_res:
- gen_pci_release_of_pci_ranges(pci);
return err;
}

-static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
+static void gen_pci_unmap_cfg(void *ptr)
+{
+ pci_ecam_free((struct pci_config_window *)ptr);
+}
+
+static struct pci_config_window *gen_pci_init(struct device *dev,
+ struct list_head *resources, struct pci_ecam_ops *ops)
{
int err;
- u8 bus_max;
- resource_size_t busn;
- struct resource *bus_range;
- struct device *dev = pci->host.dev.parent;
- struct device_node *np = dev->of_node;
- u32 sz = 1 << pci->cfg.ops->bus_shift;
+ struct resource cfgres;
+ struct resource *bus_range = NULL;
+ struct pci_config_window *cfg;

- err = of_address_to_resource(np, 0, &pci->cfg.res);
+ /* Parse our PCI ranges and request their resources */
+ err = gen_pci_parse_request_of_pci_ranges(dev, resources, &bus_range);
+ if (err)
+ goto err_out;
+
+ err = of_address_to_resource(dev->of_node, 0, &cfgres);
if (err) {
dev_err(dev, "missing \"reg\" property\n");
- return err;
+ goto err_out;
}

- /* Limit the bus-range to fit within reg */
- bus_max = pci->cfg.bus_range->start +
- (resource_size(&pci->cfg.res) >> pci->cfg.ops->bus_shift) - 1;
- pci->cfg.bus_range->end = min_t(resource_size_t,
- pci->cfg.bus_range->end, bus_max);
-
- pci->cfg.win = devm_kcalloc(dev, resource_size(pci->cfg.bus_range),
- sizeof(*pci->cfg.win), GFP_KERNEL);
- if (!pci->cfg.win)
- return -ENOMEM;
-
- /* Map our Configuration Space windows */
- if (!devm_request_mem_region(dev, pci->cfg.res.start,
- resource_size(&pci->cfg.res),
- "Configuration Space"))
- return -ENOMEM;
-
- bus_range = pci->cfg.bus_range;
- for (busn = bus_range->start; busn <= bus_range->end; ++busn) {
- u32 idx = busn - bus_range->start;
-
- pci->cfg.win[idx] = devm_ioremap(dev,
- pci->cfg.res.start + idx * sz,
- sz);
- if (!pci->cfg.win[idx])
- return -ENOMEM;
+ cfg = pci_ecam_create(dev, &cfgres, bus_range, ops);
+ if (IS_ERR(cfg)) {
+ err = PTR_ERR(cfg);
+ goto err_out;
}

- return 0;
+ err = devm_add_action(dev, gen_pci_unmap_cfg, cfg);
+ if (err) {
+ gen_pci_unmap_cfg(cfg);
+ goto err_out;
+ }
+ return cfg;
+
+err_out:
+ pci_free_resource_list(resources);
+ return ERR_PTR(err);
}

int pci_host_common_probe(struct platform_device *pdev,
- struct gen_pci *pci)
+ struct pci_ecam_ops *ops)
{
- int err;
const char *type;
struct device *dev = &pdev->dev;
struct device_node *np = dev->of_node;
struct pci_bus *bus, *child;
+ struct pci_config_window *cfg;
+ struct list_head resources;

type = of_get_property(np, "device_type", NULL);
if (!type || strcmp(type, "pci")) {
@@ -147,29 +136,18 @@ int pci_host_common_probe(struct platform_device *pdev,

of_pci_check_probe_only();

- pci->host.dev.parent = dev;
- INIT_LIST_HEAD(&pci->host.windows);
- INIT_LIST_HEAD(&pci->resources);
-
- /* Parse our PCI ranges and request their resources */
- err = gen_pci_parse_request_of_pci_ranges(pci);
- if (err)
- return err;
-
/* Parse and map our Configuration Space windows */
- err = gen_pci_parse_map_cfg_windows(pci);
- if (err) {
- gen_pci_release_of_pci_ranges(pci);
- return err;
- }
+ INIT_LIST_HEAD(&resources);
+ cfg = gen_pci_init(dev, &resources, ops);
+ if (IS_ERR(cfg))
+ return PTR_ERR(cfg);

/* Do not reassign resources if probe only */
if (!pci_has_flag(PCI_PROBE_ONLY))
pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);

-
- bus = pci_scan_root_bus(dev, pci->cfg.bus_range->start,
- &pci->cfg.ops->ops, pci, &pci->resources);
+ bus = pci_scan_root_bus(dev, cfg->busr.start, &ops->pci_ops, cfg,
+ &resources);
if (!bus) {
dev_err(dev, "Scanning rootbus failed");
return -ENODEV;
diff --git a/drivers/pci/host/pci-host-common.h b/drivers/pci/host/pci-host-common.h
deleted file mode 100644
index 09f3fa0..0000000
--- a/drivers/pci/host/pci-host-common.h
+++ /dev/null
@@ -1,47 +0,0 @@
-/*
- * 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.
- *
- * 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 for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program. If not, see <http://www.gnu.org/licenses/>.
- *
- * Copyright (C) 2014 ARM Limited
- *
- * Author: Will Deacon <***@arm.com>
- */
-
-#ifndef _PCI_HOST_COMMON_H
-#define _PCI_HOST_COMMON_H
-
-#include <linux/kernel.h>
-#include <linux/platform_device.h>
-
-struct gen_pci_cfg_bus_ops {
- u32 bus_shift;
- struct pci_ops ops;
-};
-
-struct gen_pci_cfg_windows {
- struct resource res;
- struct resource *bus_range;
- void __iomem **win;
-
- struct gen_pci_cfg_bus_ops *ops;
-};
-
-struct gen_pci {
- struct pci_host_bridge host;
- struct gen_pci_cfg_windows cfg;
- struct list_head resources;
-};
-
-int pci_host_common_probe(struct platform_device *pdev,
- struct gen_pci *pci);
-
-#endif /* _PCI_HOST_COMMON_H */
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index e8aa78f..6eaceab 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -25,41 +25,12 @@
#include <linux/of_pci.h>
#include <linux/platform_device.h>

-#include "pci-host-common.h"
+#include "../ecam.h"

-static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus,
- unsigned int devfn,
- int where)
-{
- struct gen_pci *pci = bus->sysdata;
- resource_size_t idx = bus->number - pci->cfg.bus_range->start;
-
- return pci->cfg.win[idx] + ((devfn << 8) | where);
-}
-
-static struct gen_pci_cfg_bus_ops gen_pci_cfg_cam_bus_ops = {
+static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
.bus_shift = 16,
- .ops = {
- .map_bus = gen_pci_map_cfg_bus_cam,
- .read = pci_generic_config_read,
- .write = pci_generic_config_write,
- }
-};
-
-static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus,
- unsigned int devfn,
- int where)
-{
- struct gen_pci *pci = bus->sysdata;
- resource_size_t idx = bus->number - pci->cfg.bus_range->start;
-
- return pci->cfg.win[idx] + ((devfn << 12) | where);
-}
-
-static struct gen_pci_cfg_bus_ops gen_pci_cfg_ecam_bus_ops = {
- .bus_shift = 20,
- .ops = {
- .map_bus = gen_pci_map_cfg_bus_ecam,
+ .pci_ops = {
+ .map_bus = pci_ecam_map_bus,
.read = pci_generic_config_read,
.write = pci_generic_config_write,
}
@@ -70,25 +41,22 @@ static const struct of_device_id gen_pci_of_match[] = {
.data = &gen_pci_cfg_cam_bus_ops },

{ .compatible = "pci-host-ecam-generic",
- .data = &gen_pci_cfg_ecam_bus_ops },
+ .data = &pci_generic_ecam_ops },

{ },
};
+
MODULE_DEVICE_TABLE(of, gen_pci_of_match);

static int gen_pci_probe(struct platform_device *pdev)
{
- struct device *dev = &pdev->dev;
const struct of_device_id *of_id;
- struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
-
- if (!pci)
- return -ENOMEM;
+ struct pci_ecam_ops *ops;

- of_id = of_match_node(gen_pci_of_match, dev->of_node);
- pci->cfg.ops = (struct gen_pci_cfg_bus_ops *)of_id->data;
+ of_id = of_match_node(gen_pci_of_match, pdev->dev.of_node);
+ ops = (struct pci_ecam_ops *)of_id->data;

- return pci_host_common_probe(pdev, pci);
+ return pci_host_common_probe(pdev, ops);
}

static struct platform_driver gen_pci_driver = {
diff --git a/drivers/pci/host/pci-thunder-ecam.c b/drivers/pci/host/pci-thunder-ecam.c
index d71935cb..540d030 100644
--- a/drivers/pci/host/pci-thunder-ecam.c
+++ b/drivers/pci/host/pci-thunder-ecam.c
@@ -13,18 +13,7 @@
#include <linux/of.h>
#include <linux/platform_device.h>

-#include "pci-host-common.h"
-
-/* Mapping is standard ECAM */
-static void __iomem *thunder_ecam_map_bus(struct pci_bus *bus,
- unsigned int devfn,
- int where)
-{
- struct gen_pci *pci = bus->sysdata;
- resource_size_t idx = bus->number - pci->cfg.bus_range->start;
-
- return pci->cfg.win[idx] + ((devfn << 12) | where);
-}
+#include "../ecam.h"

static void set_val(u32 v, int where, int size, u32 *val)
{
@@ -99,7 +88,7 @@ static int handle_ea_bar(u32 e0, int bar, struct pci_bus *bus,
static int thunder_ecam_p2_config_read(struct pci_bus *bus, unsigned int devfn,
int where, int size, u32 *val)
{
- struct gen_pci *pci = bus->sysdata;
+ struct pci_config_window *cfg = bus->sysdata;
int where_a = where & ~3;
void __iomem *addr;
u32 node_bits;
@@ -129,7 +118,7 @@ static int thunder_ecam_p2_config_read(struct pci_bus *bus, unsigned int devfn,
* the config space access window. Since we are working with
* the high-order 32 bits, shift everything down by 32 bits.
*/
- node_bits = (pci->cfg.res.start >> 32) & (1 << 12);
+ node_bits = (cfg->res.start >> 32) & (1 << 12);

v |= node_bits;
set_val(v, where, size, val);
@@ -358,36 +347,24 @@ static int thunder_ecam_config_write(struct pci_bus *bus, unsigned int devfn,
return pci_generic_config_write(bus, devfn, where, size, val);
}

-static struct gen_pci_cfg_bus_ops thunder_ecam_bus_ops = {
+static struct pci_ecam_ops pci_thunder_ecam_ops = {
.bus_shift = 20,
- .ops = {
- .map_bus = thunder_ecam_map_bus,
+ .pci_ops = {
+ .map_bus = pci_ecam_map_bus,
.read = thunder_ecam_config_read,
.write = thunder_ecam_config_write,
}
};

static const struct of_device_id thunder_ecam_of_match[] = {
- { .compatible = "cavium,pci-host-thunder-ecam",
- .data = &thunder_ecam_bus_ops },
-
+ { .compatible = "cavium,pci-host-thunder-ecam" },
{ },
};
MODULE_DEVICE_TABLE(of, thunder_ecam_of_match);

static int thunder_ecam_probe(struct platform_device *pdev)
{
- struct device *dev = &pdev->dev;
- const struct of_device_id *of_id;
- struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
-
- if (!pci)
- return -ENOMEM;
-
- of_id = of_match_node(thunder_ecam_of_match, dev->of_node);
- pci->cfg.ops = (struct gen_pci_cfg_bus_ops *)of_id->data;
-
- return pci_host_common_probe(pdev, pci);
+ return pci_host_common_probe(pdev, &pci_thunder_ecam_ops);
}

static struct platform_driver thunder_ecam_driver = {
diff --git a/drivers/pci/host/pci-thunder-pem.c b/drivers/pci/host/pci-thunder-pem.c
index cabb92a..a7b61ce 100644
--- a/drivers/pci/host/pci-thunder-pem.c
+++ b/drivers/pci/host/pci-thunder-pem.c
@@ -20,34 +20,22 @@
#include <linux/of_pci.h>
#include <linux/platform_device.h>

-#include "pci-host-common.h"
+#include "../ecam.h"

#define PEM_CFG_WR 0x28
#define PEM_CFG_RD 0x30

struct thunder_pem_pci {
- struct gen_pci gen_pci;
u32 ea_entry[3];
void __iomem *pem_reg_base;
};

-static void __iomem *thunder_pem_map_bus(struct pci_bus *bus,
- unsigned int devfn, int where)
-{
- struct gen_pci *pci = bus->sysdata;
- resource_size_t idx = bus->number - pci->cfg.bus_range->start;
-
- return pci->cfg.win[idx] + ((devfn << 16) | where);
-}
-
static int thunder_pem_bridge_read(struct pci_bus *bus, unsigned int devfn,
int where, int size, u32 *val)
{
u64 read_val;
- struct thunder_pem_pci *pem_pci;
- struct gen_pci *pci = bus->sysdata;
-
- pem_pci = container_of(pci, struct thunder_pem_pci, gen_pci);
+ struct pci_config_window *cfg = bus->sysdata;
+ struct thunder_pem_pci *pem_pci = (struct thunder_pem_pci *)cfg->priv;

if (devfn != 0 || where >= 2048) {
*val = ~0;
@@ -132,17 +120,17 @@ static int thunder_pem_bridge_read(struct pci_bus *bus, unsigned int devfn,
static int thunder_pem_config_read(struct pci_bus *bus, unsigned int devfn,
int where, int size, u32 *val)
{
- struct gen_pci *pci = bus->sysdata;
+ struct pci_config_window *cfg = bus->sysdata;

- if (bus->number < pci->cfg.bus_range->start ||
- bus->number > pci->cfg.bus_range->end)
+ if (bus->number < cfg->busr.start ||
+ bus->number > cfg->busr.end)
return PCIBIOS_DEVICE_NOT_FOUND;

/*
* The first device on the bus is the PEM PCIe bridge.
* Special case its config access.
*/
- if (bus->number == pci->cfg.bus_range->start)
+ if (bus->number == cfg->busr.start)
return thunder_pem_bridge_read(bus, devfn, where, size, val);

return pci_generic_config_read(bus, devfn, where, size, val);
@@ -187,12 +175,11 @@ static u32 thunder_pem_bridge_w1c_bits(int where)
static int thunder_pem_bridge_write(struct pci_bus *bus, unsigned int devfn,
int where, int size, u32 val)
{
- struct gen_pci *pci = bus->sysdata;
- struct thunder_pem_pci *pem_pci;
+ struct pci_config_window *cfg = bus->sysdata;
+ struct thunder_pem_pci *pem_pci = (struct thunder_pem_pci *)cfg->priv;
u64 write_val, read_val;
u32 mask = 0;

- pem_pci = container_of(pci, struct thunder_pem_pci, gen_pci);

if (devfn != 0 || where >= 2048)
return PCIBIOS_DEVICE_NOT_FOUND;
@@ -256,53 +243,38 @@ static int thunder_pem_bridge_write(struct pci_bus *bus, unsigned int devfn,
static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
int where, int size, u32 val)
{
- struct gen_pci *pci = bus->sysdata;
+ struct pci_config_window *cfg = bus->sysdata;

- if (bus->number < pci->cfg.bus_range->start ||
- bus->number > pci->cfg.bus_range->end)
+ if (bus->number < cfg->busr.start ||
+ bus->number > cfg->busr.end)
return PCIBIOS_DEVICE_NOT_FOUND;
/*
* The first device on the bus is the PEM PCIe bridge.
* Special case its config access.
*/
- if (bus->number == pci->cfg.bus_range->start)
+ if (bus->number == cfg->busr.start)
return thunder_pem_bridge_write(bus, devfn, where, size, val);

return pci_generic_config_write(bus, devfn, where, size, val);
}

-static struct gen_pci_cfg_bus_ops thunder_pem_bus_ops = {
- .bus_shift = 24,
- .ops = {
- .map_bus = thunder_pem_map_bus,
- .read = thunder_pem_config_read,
- .write = thunder_pem_config_write,
- }
-};
-
-static const struct of_device_id thunder_pem_of_match[] = {
- { .compatible = "cavium,pci-host-thunder-pem",
- .data = &thunder_pem_bus_ops },
-
- { },
-};
-MODULE_DEVICE_TABLE(of, thunder_pem_of_match);
-
-static int thunder_pem_probe(struct platform_device *pdev)
+static int thunder_pem_init(struct device *dev, struct pci_config_window *cfg)
{
- struct device *dev = &pdev->dev;
- const struct of_device_id *of_id;
resource_size_t bar4_start;
struct resource *res_pem;
struct thunder_pem_pci *pem_pci;
+ struct platform_device *pdev;
+
+ /* Only OF support for now */
+ if (!dev->of_node)
+ return -EINVAL;

pem_pci = devm_kzalloc(dev, sizeof(*pem_pci), GFP_KERNEL);
if (!pem_pci)
return -ENOMEM;

- of_id = of_match_node(thunder_pem_of_match, dev->of_node);
- pem_pci->gen_pci.cfg.ops = (struct gen_pci_cfg_bus_ops *)of_id->data;
+ pdev = to_platform_device(dev);

/*
* The second register range is the PEM bridge to the PCIe
@@ -330,7 +302,29 @@ static int thunder_pem_probe(struct platform_device *pdev)
pem_pci->ea_entry[1] = (u32)(res_pem->end - bar4_start) & ~3u;
pem_pci->ea_entry[2] = (u32)(bar4_start >> 32);

- return pci_host_common_probe(pdev, &pem_pci->gen_pci);
+ cfg->priv = pem_pci;
+ return 0;
+}
+
+static struct pci_ecam_ops pci_thunder_pem_ops = {
+ .bus_shift = 24,
+ .init = thunder_pem_init,
+ .pci_ops = {
+ .map_bus = pci_ecam_map_bus,
+ .read = thunder_pem_config_read,
+ .write = thunder_pem_config_write,
+ }
+};
+
+static const struct of_device_id thunder_pem_of_match[] = {
+ { .compatible = "cavium,pci-host-thunder-pem" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, thunder_pem_of_match);
+
+static int thunder_pem_probe(struct platform_device *pdev)
+{
+ return pci_host_common_probe(pdev, &pci_thunder_pem_ops);
}

static struct platform_driver thunder_pem_driver = {

--
1.9.1

Tomasz Nowicki

2016-05-10 15:20:35 UTC

ACPI requires to run acpi_pci_{add|remove}_bus while new PCI bus is created.
This allows to do some ACPI-specific additional configuration, like
PCI hotplug slot enumeration. In order to fulfill these requirements,
we implement arch-specific pcibios_{add|remove}_bus calls
and call acpi_pci_{add|remove}_bus from there.

Signed-off-by: Tomasz Nowicki <***@semihalf.com>
To: Catalin Marinas <***@arm.com>
To: Lorenzo Pieralisi <***@arm.com>
To: Will Deacon <***@arm.com>
To: Arnd Bergmann <***@arndb.de>
---
arch/arm64/kernel/pci.c | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index 15109c11..eeec5f6 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -17,6 +17,7 @@
#include <linux/mm.h>
#include <linux/of_pci.h>
#include <linux/of_platform.h>
+#include <linux/pci-acpi.h>
#include <linux/slab.h>

/*
@@ -64,6 +65,16 @@ int pcibios_alloc_irq(struct pci_dev *dev)
return 0;
}

+void pcibios_add_bus(struct pci_bus *bus)
+{
+ acpi_pci_add_bus(bus);
+}
+
+void pcibios_remove_bus(struct pci_bus *bus)
+{
+ acpi_pci_remove_bus(bus);
+}
+
/*
* raw_pci_read/write - Platform-specific PCI config space access.
*/

--
1.9.1

Tomasz Nowicki

2016-05-10 15:20:53 UTC

Platforms that have memory mapped IO port (such as ARM64) need special
handling for PCI I/O resources. For host bridge's resource probing case
these resources need to be fixed up with pci_register_io_range/pci_remap_iospace etc.

The same I/O resources need to be released after hotplug
removal so that it can be re-added back by the pci_remap_iospace
function during insertion. As a consequence we unmap I/O resources
with pci_unmap_iospace when we release host bridge resources.

Signed-off-by: Jayachandran C <***@broadcom.com>
Signed-off-by: Sinan Kaya <***@codeaurora.org>
Signed-off-by: Tomasz Nowicki <***@semihalf.com>
---
drivers/acpi/pci_root.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index ae3fe4e..cb3071d 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -719,6 +719,34 @@ next:
resource_list_add_tail(entry, resources);
}
}
+static void acpi_pci_root_remap_iospace(struct resource_entry *entry)
+{
+#ifdef PCI_IOBASE
+ struct resource *res = entry->res;
+ resource_size_t cpu_addr = res->start;
+ resource_size_t pci_addr = cpu_addr - entry->offset;
+ resource_size_t length = resource_size(res);
+ unsigned long port;
+
+ if (pci_register_io_range(cpu_addr, length))
+ goto err;
+
+ port = pci_address_to_pio(cpu_addr);
+ if (port == (unsigned long)-1)
+ goto err;
+
+ res->start = port;
+ res->end = port + length - 1;
+ entry->offset = port - pci_addr;
+
+ if (pci_remap_iospace(res, cpu_addr) < 0)
+ goto err;
+ pr_info("Remapped I/O %pa to %pR\n", &cpu_addr, res);
+ return;
+err:
+ res->flags |= IORESOURCE_DISABLED;
+#endif
+}

int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
{
@@ -740,6 +768,9 @@ int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
"no IO and memory resources present in _CRS\n");
else {
resource_list_for_each_entry_safe(entry, tmp, list) {
+ if (entry->res->flags & IORESOURCE_IO)
+ acpi_pci_root_remap_iospace(entry);
+
if (entry->res->flags & IORESOURCE_DISABLED)
resource_list_destroy_entry(entry);
else
@@ -811,6 +842,8 @@ static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)

resource_list_for_each_entry(entry, &bridge->windows) {
res = entry->res;
+ if (res->flags & IORESOURCE_IO)
+ pci_unmap_iospace(res);
if (res->parent &&
(res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
release_resource(res);

--
1.9.1

Rafael J. Wysocki

2016-05-10 18:20:41 UTC

Post by Tomasz Nowicki
Platforms that have memory mapped IO port (such as ARM64) need special
handling for PCI I/O resources. For host bridge's resource probing case
these resources need to be fixed up with pci_register_io_range/pci_remap_iospace etc.
The same I/O resources need to be released after hotplug
removal so that it can be re-added back by the pci_remap_iospace
function during insertion. As a consequence we unmap I/O resources
with pci_unmap_iospace when we release host bridge resources.
---
drivers/acpi/pci_root.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index ae3fe4e..cb3071d 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
resource_list_add_tail(entry, resources);
}
}
+static void acpi_pci_root_remap_iospace(struct resource_entry *entry)
+{
+#ifdef PCI_IOBASE

Same comment about the #ifdefs as in the other patch.

Post by Tomasz Nowicki
+ struct resource *res = entry->res;
+ resource_size_t cpu_addr = res->start;
+ resource_size_t pci_addr = cpu_addr - entry->offset;
+ resource_size_t length = resource_size(res);
+ unsigned long port;
+
+ if (pci_register_io_range(cpu_addr, length))
+ goto err;
+
+ port = pci_address_to_pio(cpu_addr);
+ if (port == (unsigned long)-1)
+ goto err;
+
+ res->start = port;
+ res->end = port + length - 1;
+ entry->offset = port - pci_addr;
+
+ if (pci_remap_iospace(res, cpu_addr) < 0)
+ goto err;

An empty line here?

Post by Tomasz Nowicki
+ pr_info("Remapped I/O %pa to %pR\n", &cpu_addr, res);
+ return;
+ res->flags |= IORESOURCE_DISABLED;
+#endif
+}
int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
{
@@ -740,6 +768,9 @@ int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
"no IO and memory resources present in _CRS\n");
else {
resource_list_for_each_entry_safe(entry, tmp, list) {
+ if (entry->res->flags & IORESOURCE_IO)
+ acpi_pci_root_remap_iospace(entry);
+
if (entry->res->flags & IORESOURCE_DISABLED)
resource_list_destroy_entry(entry);
else
@@ -811,6 +842,8 @@ static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
resource_list_for_each_entry(entry, &bridge->windows) {
res = entry->res;
+ if (res->flags & IORESOURCE_IO)
+ pci_unmap_iospace(res);
if (res->parent &&
(res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
release_resource(res);
--

Tomasz Nowicki

2016-05-11 07:39:45 UTC

Same comment about the #ifdefs as in the other patch.

An empty line here?

yes, empty line would be nice here.

Tomasz

Tomasz Nowicki

2016-05-10 15:21:07 UTC

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.

Signed-off-by: Tomasz Nowicki <***@semihalf.com>
Signed-off-by: Jayachandran C <***@broadcom.com>
---
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 <***@broadcom.com>
+ * Copyright (C) 2016 Semihalf
+ * Author: Tomasz Nowicki <***@semihalf.com>
+ *
+ * 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))
+
+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;
+ }
+ }
+
+ 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;
+
+ return pci_mcfg_lookup_static(seg, bus_res->start, bus_res->end);
+}
+
+static __init int pci_mcfg_parse(struct acpi_table_header *header)
+{
+ struct acpi_table_mcfg *mcfg;
+ int n;
+
+ if (!header)
+ return -EINVAL;
+
+ 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;
+ 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);
+}
diff --git a/drivers/acpi/pci_root_generic.c b/drivers/acpi/pci_root_generic.c
new file mode 100644
index 0000000..6f4940a
--- /dev/null
+++ b/drivers/acpi/pci_root_generic.c
@@ -0,0 +1,149 @@
+/*
+ * Copyright (C) 2016 Broadcom
+ * Author: Jayachandran C <***@broadcom.com>
+ * Copyright (C) 2016 Semihalf
+ * Author: Tomasz Nowicki <***@semihalf.com>
+ *
+ * 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/slab.h>
+
+#include "../pci/ecam.h"
+
+#define PREFIX "ACPI PCI: "
+
+/* 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 */
+};
+
+void acpi_pci_set_companion(struct pci_host_bridge *bridge)
+{
+ struct pci_config_window *cfg = bridge->bus->sysdata;
+
+ ACPI_COMPANION_SET(&bridge->dev, cfg->companion);
+}
+
+int acpi_pci_bus_domain_nr(struct pci_bus *bus)
+{
+ struct pci_config_window *cfg = bus->sysdata;
+
+ return cfg->domain;
+}
+
+/*
+ * 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)
+{
+ struct resource *bus_res = &root->secondary;
+ u16 seg = root->segment;
+ struct pci_config_window *cfg;
+ struct resource cfgres;
+ unsigned int bsz;
+ phys_addr_t addr;
+
+ addr = pci_mcfg_lookup(root->device, seg, bus_res);
+ if (IS_ERR_VALUE(addr)) {
+ pr_err(PREFIX"%04x:%pR MCFG region not found\n", seg, bus_res);
+ return addr;
+ }
+
+ bsz = 1 << pci_generic_ecam_ops.bus_shift;
+ cfgres.start = addr + bus_res->start * bsz;
+ cfgres.end = addr + (bus_res->end + 1) * bsz - 1;
+ cfgres.flags = IORESOURCE_MEM;
+ cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res,
+ &pci_generic_ecam_ops);
+ if (IS_ERR(cfg)) {
+ pr_err("%04x:%pR error %ld mapping CAM\n", seg, bus_res,
+ PTR_ERR(cfg));
+ return PTR_ERR(cfg);
+ }
+
+ cfg->domain = seg;
+ cfg->companion = root->device;
+ ri->cfg = cfg;
+ return 0;
+}
+
+/* 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_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;
+}
+
+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/drivers/pci/ecam.h b/drivers/pci/ecam.h
index 1ad2176..1cccf57 100644
--- a/drivers/pci/ecam.h
+++ b/drivers/pci/ecam.h
@@ -45,6 +45,11 @@ struct pci_config_window {
void __iomem *win; /* 64-bit single mapping */
void __iomem **winp; /* 32-bit per bus mapping */
};
+#ifdef CONFIG_ACPI_PCI_HOST_GENERIC
+ struct acpi_device *companion; /* ACPI companion device */
+#endif
+ int domain;
+
};

/* create and free for pci_config_window */
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 1baa515..42ff844 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -111,6 +111,10 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
#endif /* CONFIG_ACPI */

+#ifdef CONFIG_ACPI_PCI_HOST_GENERIC
+void acpi_pci_set_companion(struct pci_host_bridge *bridge);
+int acpi_pci_bus_domain_nr(struct pci_bus *bus);
+#else
static inline void acpi_pci_set_companion(struct pci_host_bridge *bridge)
{
}
@@ -119,6 +123,7 @@ static inline int acpi_pci_bus_domain_nr(struct pci_bus *bus)
{
return 0;
}
+#endif

#ifdef CONFIG_ACPI_APEI
extern bool aer_acpi_firmware_first(void);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d6ea6ce..b2e8886 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1722,7 +1722,10 @@ void pcibios_free_irq(struct pci_dev *dev);
extern struct dev_pm_ops pcibios_pm_ops;
#endif

-#ifdef CONFIG_PCI_MMCONFIG
+#if defined(CONFIG_PCI_MMCONFIG) || defined(CONFIG_ACPI_PCI_HOST_GENERIC)
+struct acpi_device;
+phys_addr_t pci_mcfg_lookup(struct acpi_device *device, u16 seg,
+ struct resource *bus_res);
void __init pci_mmcfg_early_init(void);
void __init pci_mmcfg_late_init(void);
#else

--
1.9.1

Rafael J. Wysocki

2016-05-10 17:54:26 UTC

Why do we need a new file? Would there be any problem with adding
that code to pci_root.c?

Rafael J. Wysocki

2016-05-10 18:18:52 UTC

Does it mean x86 and ia64 will now be able to use this code too?

Post by Tomasz Nowicki
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.

If that code really is generic and there will be more than one
architecture using it ever, I think it'll be better for the
architectures that don't use it to set something like
ARCH_ACPI_PCI_HOST and whoever doesn't set that will use the generic
thing. That'd be more logical at least IMO.

Post by Tomasz Nowicki
---
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
+ * Copyright (C) 2016 Semihalf
+ *
+ * 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: "

If that is a new file (and I'm totally unconvinced about the need for
it), can we simply define a pr_fmt() here as all messages in it seem
to be printed by the pr_* functions?

Jayachandran C

2016-05-13 11:26:37 UTC

[....]

Post by Tomasz Nowicki
diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h
index 1ad2176..1cccf57 100644
--- a/drivers/pci/ecam.h
+++ b/drivers/pci/ecam.h
@@ -45,6 +45,11 @@ struct pci_config_window {
void __iomem *win; /* 64-bit single mapping */
void __iomem **winp; /* 32-bit per bus mapping */
};
+#ifdef CONFIG_ACPI_PCI_HOST_GENERIC
+ struct acpi_device *companion; /* ACPI companion device */
+#endif
+ int domain;
+
};

Using struct pci_config_window to pass along domain and
companion looks bad. I think there are two possible options
to do this better:

1. add a 'struct fwnode_handle *' or 'struct device *parent_dev'
instead of the companion and domain fields above. In case of
ACPI either of them can be used to get the acpi_device and
both domain and companion can be set from that.

2. make pci_config_window fully embeddable by moving allocation
out of pci_ecam_create to its callers. Then it can be embedded
into acpi_pci_generic_root_info, and container_of can be used
to get acpi info from ->sysdata.

The first option should be easier to implement but the second may
be better on long run. I would leave it to the Bjorn or Rafael to
suggest which is preferred.

JC.

Rafael J. Wysocki

2016-05-13 11:31:40 UTC

[....]

Using struct pci_config_window to pass along domain and
companion looks bad. I think there are two possible options
1. add a 'struct fwnode_handle *' or 'struct device *parent_dev'
instead of the companion and domain fields above. In case of
ACPI either of them can be used to get the acpi_device and
both domain and companion can be set from that.
2. make pci_config_window fully embeddable by moving allocation
out of pci_ecam_create to its callers. Then it can be embedded
into acpi_pci_generic_root_info, and container_of can be used
to get acpi info from ->sysdata.
The first option should be easier to implement but the second may
be better on long run. I would leave it to the Bjorn or Rafael to
suggest which is preferred.

Personally, I'd probably try to use fwnode_handle, but the second
option makes sense too in principle.

Tomasz Nowicki

2016-05-13 11:42:22 UTC

[....]

Personally, I'd probably try to use fwnode_handle, but the second
option makes sense too in principle.

Thanks for suggestions. I will try to use fwnode_handle

Tomasz

Jayachandran C

2016-05-14 09:08:39 UTC

Looking thru the new code, I see a few issues, please see below

It would be better if you used static inline function here.

Post by Tomasz Nowicki
+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.

Post by Tomasz Nowicki
+ 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.

Post by Tomasz Nowicki
+
+ 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.

Post by Tomasz Nowicki
+}
+
+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

Post by Tomasz Nowicki
+
+ 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.

Post by Tomasz Nowicki
+ 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.

Tomasz Nowicki

2016-05-23 11:34:24 UTC

Looking thru the new code, I see a few issues, please see below

It would be better if you used static inline function here.

Yeah we can implement algorithm like that but IMO it would try to guess
what FW designer meant putting various combination of BUS resources into
the tables. We should rather be strict about this so I agree with Bjorn:
https://lkml.org/lkml/2016/4/28/807
Either we get exact match or MCFG covers more than the host bridge range.

Host bridge bus range definition -> MCFG
_BBN-0xff -> we expect exact _BBN-0xff match
0-0xff -> ditto
_CRS (the only way to trim host bridge bus rage) -> exact match or MCFG
covers more

Post by Jayachandran C
Also (trivial), the braces are not needed.

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?

I think so, see above. Maybe Bjorn may advise here.

Post by Jayachandran C
(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.

Post by Tomasz Nowicki
+
+ 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.

Post by Tomasz Nowicki
+}
+
+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

You got the point about early_acpi_os_unmap_memory but that would be the
case if we call pci_mmcfg_late_init before acpi_early_init. But
pci_mmcfg_late_init is called much later. Also the code is much simpler
as is now.

Thanks,
Tomasz

Matthias Brugger

2016-05-19 16:56:49 UTC

Post by Tomasz Nowicki
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.
---
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
+ * Copyright (C) 2016 Semihalf
+ *
+ * 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))
+
+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;
+ }
+ }
+
+ 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;
+
+ return pci_mcfg_lookup_static(seg, bus_res->start, bus_res->end);
+}
+
+static __init int pci_mcfg_parse(struct acpi_table_header *header)
+{
+ struct acpi_table_mcfg *mcfg;
+ int n;
+
+ if (!header)
+ return -EINVAL;

Maybe that's bike shedding, but acpi_table_parse already checks that the
table passed to the handler exists, so this is redundant.

Regards,
Matthias

Tomasz Nowicki

2016-05-10 15:21:45 UTC

This patch provides a way to set the ACPI domain number in PCI code.
pci_create_root_bus is called with NULL as parent in ACPI. This
ends up calling pci_bus_assign_domain_nr with a NULL parent.
So we define acpi_pci_bus_domain_nr() which is meant to retrieve
the PCI domain number based on 'struct pci_bus' in the ACPI way.
pci_bus_assign_domain_nr() is updated to call acpi_pci_bus_domain_nr()
and assign domain number on the root bus, in case of ACPI.

acpi_pci_bus_domain_nr function is stub for now.

While at it, for the sake of code clarity we put ACPI and DT domain
assign methods into the corresponding helpers.

Signed-off-by: Jayachandran C <***@broadcom.com>
Signed-off-by: Tomasz Nowicki <***@semihalf.com>
---
drivers/pci/pci.c | 11 +++++++++--
include/linux/pci-acpi.h | 5 +++++
2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ff97a0b..a1d7bcf 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -19,6 +19,7 @@
#include <linux/spinlock.h>
#include <linux/string.h>
#include <linux/log2.h>
+#include <linux/pci-acpi.h>
#include <linux/pci-aspm.h>
#include <linux/pm_wakeup.h>
#include <linux/interrupt.h>
@@ -4918,7 +4919,7 @@ int pci_get_new_domain_nr(void)
}

#ifdef CONFIG_PCI_DOMAINS_GENERIC
-void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
+static int of_pci_bus_domain_nr(struct device *parent)
{
static int use_dt_domains = -1;
int domain = -1;
@@ -4962,7 +4963,13 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
domain = -1;
}

- bus->domain_nr = domain;
+ return domain;
+}
+
+void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
+{
+ bus->domain_nr = acpi_disabled ? of_pci_bus_domain_nr(parent) :
+ acpi_pci_bus_domain_nr(bus);
}
#endif
#endif
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 89ab057..09f9f02 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -111,6 +111,11 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
#endif /* CONFIG_ACPI */

+static inline int acpi_pci_bus_domain_nr(struct pci_bus *bus)
+{
+ return 0;
+}
+
#ifdef CONFIG_ACPI_APEI
extern bool aer_acpi_firmware_first(void);
#else

--
1.9.1

Tomasz Nowicki

2016-05-10 15:21:47 UTC

It is perfectly fine to use ACPI_PCI_HOST_GENERIC for ARM64,
so lets get rid of PCI init and RAW ACPI accessor empty stubs
and go with full-blown PCI host controller driver.

Signed-off-by: Tomasz Nowicki <***@semihalf.com>
To: Catalin Marinas <***@arm.com>
To: Lorenzo Pieralisi <***@arm.com>
To: Will Deacon <***@arm.com>
To: Arnd Bergmann <***@arndb.de>
---
arch/arm64/Kconfig | 1 +
arch/arm64/kernel/pci.c | 24 ------------------------
2 files changed, 1 insertion(+), 24 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 4f43622..1bded87 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -2,6 +2,7 @@ config ARM64
def_bool y
select ACPI_CCA_REQUIRED if ACPI
select ACPI_GENERIC_GSI if ACPI
+ select ACPI_PCI_HOST_GENERIC if ACPI
select ACPI_REDUCED_HARDWARE_ONLY if ACPI
select ARCH_HAS_DEVMEM_IS_ALLOWED
select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index eeec5f6..e484c91 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -74,27 +74,3 @@ void pcibios_remove_bus(struct pci_bus *bus)
{
acpi_pci_remove_bus(bus);
}
-
-/*
- * raw_pci_read/write - Platform-specific PCI config space access.
- */
-int raw_pci_read(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 *val)
-{
- return -ENXIO;
-}
-
-int raw_pci_write(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 val)
-{
- return -ENXIO;
-}
-
-#ifdef CONFIG_ACPI
-/* Root bridge scanning */
-struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
-{
- /* TODO: Should be revisited when implementing PCI on ACPI */
- return NULL;
-}
-#endif

--
1.9.1

Tomasz Nowicki

2016-05-10 15:22:20 UTC

To enable PCI legacy IRQs on platforms booting with ACPI, arch code
should include ACPI specific callbacks that parse and set-up the
device IRQ number, equivalent to the DT boot path. Owing to the current
ACPI core scan handlers implementation, ACPI PCI legacy IRQs bindings
cannot be parsed at device add time, since that would trigger ACPI scan
handlers ordering issues depending on how the ACPI tables are defined.

To solve this problem and consolidate FW PCI legacy IRQs parsing in
one single pcibios callback (pending final removal), this patch moves
DT PCI IRQ parsing to the pcibios_alloc_irq() callback (called by
PCI core code at device probe time) and adds ACPI PCI legacy IRQs
parsing to the same callback too, so that FW PCI legacy IRQs parsing
is confined in one single arch callback that can be easily removed
when code parsing PCI legacy IRQs is consolidated and moved to core
PCI code.

Signed-off-by: Tomasz Nowicki <***@semihalf.com>
Suggested-by: Lorenzo Pieralisi <***@arm.com>
---
arch/arm64/kernel/pci.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index c72de66..15109c11 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -50,11 +50,16 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
}

/*
- * Try to assign the IRQ number from DT when adding a new device
+ * Try to assign the IRQ number when probing a new device
*/
-int pcibios_add_device(struct pci_dev *dev)
+int pcibios_alloc_irq(struct pci_dev *dev)
{
- dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+ if (acpi_disabled)
+ dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+#ifdef CONFIG_ACPI
+ else
+ return acpi_pci_irq_enable(dev);
+#endif

return 0;
}

--
1.9.1

Tomasz Nowicki

2016-05-10 15:22:45 UTC

No functional changes in this patch.

PCI I/O space mapping code does not depend on OF, therefore it can be
moved to PCI core code. This way we will be able to use it
e.g. in ACPI PCI code.

Suggested-by: Lorenzo Pieralisi <***@arm.com>
Signed-off-by: Tomasz Nowicki <***@semihalf.com>
CC: Arnd Bergmann <***@arndb.de>
CC: Liviu Dudau <***@arm.com>
---
drivers/of/address.c | 116 +--------------------------------------------
drivers/pci/pci.c | 115 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/of_address.h | 9 ----
include/linux/pci.h | 5 ++
4 files changed, 121 insertions(+), 124 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 91a469d..0a553c0 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -4,6 +4,7 @@
#include <linux/ioport.h>
#include <linux/module.h>
#include <linux/of_address.h>
+#include <linux/pci.h>
#include <linux/pci_regs.h>
#include <linux/sizes.h>
#include <linux/slab.h>
@@ -673,121 +674,6 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
}
EXPORT_SYMBOL(of_get_address);

-#ifdef PCI_IOBASE
-struct io_range {
- struct list_head list;
- phys_addr_t start;
- resource_size_t size;
-};
-
-static LIST_HEAD(io_range_list);
-static DEFINE_SPINLOCK(io_range_lock);
-#endif
-
-/*
- * Record the PCI IO range (expressed as CPU physical address + size).
- * Return a negative value if an error has occured, zero otherwise
- */
-int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
-{
- int err = 0;
-
-#ifdef PCI_IOBASE
- struct io_range *range;
- resource_size_t allocated_size = 0;
-
- /* check if the range hasn't been previously recorded */
- spin_lock(&io_range_lock);
- list_for_each_entry(range, &io_range_list, list) {
- if (addr >= range->start && addr + size <= range->start + size) {
- /* range already registered, bail out */
- goto end_register;
- }
- allocated_size += range->size;
- }
-
- /* range not registed yet, check for available space */
- if (allocated_size + size - 1 > IO_SPACE_LIMIT) {
- /* if it's too big check if 64K space can be reserved */
- if (allocated_size + SZ_64K - 1 > IO_SPACE_LIMIT) {
- err = -E2BIG;
- goto end_register;
- }
-
- size = SZ_64K;
- pr_warn("Requested IO range too big, new size set to 64K\n");
- }
-
- /* add the range to the list */
- range = kzalloc(sizeof(*range), GFP_ATOMIC);
- if (!range) {
- err = -ENOMEM;
- goto end_register;
- }
-
- range->start = addr;
- range->size = size;
-
- list_add_tail(&range->list, &io_range_list);
-
-end_register:
- spin_unlock(&io_range_lock);
-#endif
-
- return err;
-}
-
-phys_addr_t pci_pio_to_address(unsigned long pio)
-{
- phys_addr_t address = (phys_addr_t)OF_BAD_ADDR;
-
-#ifdef PCI_IOBASE
- struct io_range *range;
- resource_size_t allocated_size = 0;
-
- if (pio > IO_SPACE_LIMIT)
- return address;
-
- spin_lock(&io_range_lock);
- list_for_each_entry(range, &io_range_list, list) {
- if (pio >= allocated_size && pio < allocated_size + range->size) {
- address = range->start + pio - allocated_size;
- break;
- }
- allocated_size += range->size;
- }
- spin_unlock(&io_range_lock);
-#endif
-
- return address;
-}
-
-unsigned long __weak pci_address_to_pio(phys_addr_t address)
-{
-#ifdef PCI_IOBASE
- struct io_range *res;
- resource_size_t offset = 0;
- unsigned long addr = -1;
-
- spin_lock(&io_range_lock);
- list_for_each_entry(res, &io_range_list, list) {
- if (address >= res->start && address < res->start + res->size) {
- addr = address - res->start + offset;
- break;
- }
- offset += res->size;
- }
- spin_unlock(&io_range_lock);
-
- return addr;
-#else
- if (address > IO_SPACE_LIMIT)
- return (unsigned long)-1;
-
- return (unsigned long) address;
-#endif
-}
-
static int __of_address_to_resource(struct device_node *dev,
const __be32 *addrp, u64 size, unsigned int flags,
const char *name, struct resource *r)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 25e0327..bc0c914 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3021,6 +3021,121 @@ int pci_request_regions_exclusive(struct pci_dev *pdev, const char *res_name)
}
EXPORT_SYMBOL(pci_request_regions_exclusive);

+#ifdef PCI_IOBASE
+struct io_range {
+ struct list_head list;
+ phys_addr_t start;
+ resource_size_t size;
+};
+
+static LIST_HEAD(io_range_list);
+static DEFINE_SPINLOCK(io_range_lock);
+#endif
+
+/*
+ * Record the PCI IO range (expressed as CPU physical address + size).
+ * Return a negative value if an error has occured, zero otherwise
+ */
+int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
+{
+ int err = 0;
+
+#ifdef PCI_IOBASE
+ struct io_range *range;
+ resource_size_t allocated_size = 0;
+
+ /* check if the range hasn't been previously recorded */
+ spin_lock(&io_range_lock);
+ list_for_each_entry(range, &io_range_list, list) {
+ if (addr >= range->start && addr + size <= range->start + size) {
+ /* range already registered, bail out */
+ goto end_register;
+ }
+ allocated_size += range->size;
+ }
+
+ /* range not registed yet, check for available space */
+ if (allocated_size + size - 1 > IO_SPACE_LIMIT) {
+ /* if it's too big check if 64K space can be reserved */
+ if (allocated_size + SZ_64K - 1 > IO_SPACE_LIMIT) {
+ err = -E2BIG;
+ goto end_register;
+ }
+
+ size = SZ_64K;
+ pr_warn("Requested IO range too big, new size set to 64K\n");
+ }
+
+ /* add the range to the list */
+ range = kzalloc(sizeof(*range), GFP_ATOMIC);
+ if (!range) {
+ err = -ENOMEM;
+ goto end_register;
+ }
+
+ range->start = addr;
+ range->size = size;
+
+ list_add_tail(&range->list, &io_range_list);
+
+end_register:
+ spin_unlock(&io_range_lock);
+#endif
+
+ return err;
+}
+
+phys_addr_t pci_pio_to_address(unsigned long pio)
+{
+ phys_addr_t address = (phys_addr_t)OF_BAD_ADDR;
+
+#ifdef PCI_IOBASE
+ struct io_range *range;
+ resource_size_t allocated_size = 0;
+
+ if (pio > IO_SPACE_LIMIT)
+ return address;
+
+ spin_lock(&io_range_lock);
+ list_for_each_entry(range, &io_range_list, list) {
+ if (pio >= allocated_size && pio < allocated_size + range->size) {
+ address = range->start + pio - allocated_size;
+ break;
+ }
+ allocated_size += range->size;
+ }
+ spin_unlock(&io_range_lock);
+#endif
+
+ return address;
+}
+
+unsigned long __weak pci_address_to_pio(phys_addr_t address)
+{
+#ifdef PCI_IOBASE
+ struct io_range *res;
+ resource_size_t offset = 0;
+ unsigned long addr = -1;
+
+ spin_lock(&io_range_lock);
+ list_for_each_entry(res, &io_range_list, list) {
+ if (address >= res->start && address < res->start + res->size) {
+ addr = address - res->start + offset;
+ break;
+ }
+ offset += res->size;
+ }
+ spin_unlock(&io_range_lock);
+
+ return addr;
+#else
+ if (address > IO_SPACE_LIMIT)
+ return (unsigned long)-1;
+
+ return (unsigned long) address;
+#endif
+}
+
/**
* pci_remap_iospace - Remap the memory mapped I/O space
* @res: Resource describing the I/O space
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 01c0a55..3786473 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -47,10 +47,6 @@ void __iomem *of_io_request_and_map(struct device_node *device,
extern const __be32 *of_get_address(struct device_node *dev, int index,
u64 *size, unsigned int *flags);

-extern int pci_register_io_range(phys_addr_t addr, resource_size_t size);
-extern unsigned long pci_address_to_pio(phys_addr_t addr);
-extern phys_addr_t pci_pio_to_address(unsigned long pio);
-
extern int of_pci_range_parser_init(struct of_pci_range_parser *parser,
struct device_node *node);
extern struct of_pci_range *of_pci_range_parser_one(
@@ -86,11 +82,6 @@ static inline const __be32 *of_get_address(struct device_node *dev, int index,
return NULL;
}

-static inline phys_addr_t pci_pio_to_address(unsigned long pio)
-{
- return 0;
-}
-
static inline int of_pci_range_parser_init(struct of_pci_range_parser *parser,
struct device_node *node)
{
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 932ec74..d2a57f3 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1165,6 +1165,9 @@ int __must_check pci_bus_alloc_resource(struct pci_bus *bus,
void *alignf_data);

+int pci_register_io_range(phys_addr_t addr, resource_size_t size);
+unsigned long pci_address_to_pio(phys_addr_t addr);
+phys_addr_t pci_pio_to_address(unsigned long pio);
int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr);

static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
@@ -1481,6 +1484,8 @@ static inline int pci_request_regions(struct pci_dev *dev, const char *res_name)
{ return -EIO; }
static inline void pci_release_regions(struct pci_dev *dev) { }

+static inline unsigned long pci_address_to_pio(phys_addr_t addr) { return -1; }
+
static inline void pci_block_cfg_access(struct pci_dev *dev) { }
static inline int pci_block_cfg_access_in_atomic(struct pci_dev *dev)
{ return 0; }

--
1.9.1

Rafael J. Wysocki

2016-05-10 17:59:15 UTC

Post by Tomasz Nowicki
No functional changes in this patch.
PCI I/O space mapping code does not depend on OF, therefore it can be
moved to PCI core code. This way we will be able to use it
e.g. in ACPI PCI code.
---
drivers/of/address.c | 116 +--------------------------------------------
drivers/pci/pci.c | 115 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/of_address.h | 9 ----
include/linux/pci.h | 5 ++
4 files changed, 121 insertions(+), 124 deletions(-)
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 91a469d..0a553c0 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -4,6 +4,7 @@
#include <linux/ioport.h>
#include <linux/module.h>
#include <linux/of_address.h>
+#include <linux/pci.h>
#include <linux/pci_regs.h>
#include <linux/sizes.h>
#include <linux/slab.h>
@@ -673,121 +674,6 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
}
EXPORT_SYMBOL(of_get_address);
-#ifdef PCI_IOBASE
-struct io_range {
- struct list_head list;
- phys_addr_t start;
- resource_size_t size;
-};
-
-static LIST_HEAD(io_range_list);
-static DEFINE_SPINLOCK(io_range_lock);
-#endif
-
-/*
- * Record the PCI IO range (expressed as CPU physical address + size).
- * Return a negative value if an error has occured, zero otherwise
- */
-int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
-{
- int err = 0;
-
-#ifdef PCI_IOBASE
- struct io_range *range;
- resource_size_t allocated_size = 0;
-
- /* check if the range hasn't been previously recorded */
- spin_lock(&io_range_lock);
- list_for_each_entry(range, &io_range_list, list) {
- if (addr >= range->start && addr + size <= range->start + size) {
- /* range already registered, bail out */
- goto end_register;
- }
- allocated_size += range->size;
- }
-
- /* range not registed yet, check for available space */
- if (allocated_size + size - 1 > IO_SPACE_LIMIT) {
- /* if it's too big check if 64K space can be reserved */
- if (allocated_size + SZ_64K - 1 > IO_SPACE_LIMIT) {
- err = -E2BIG;
- goto end_register;
- }
-
- size = SZ_64K;
- pr_warn("Requested IO range too big, new size set to 64K\n");
- }
-
- /* add the range to the list */
- range = kzalloc(sizeof(*range), GFP_ATOMIC);
- if (!range) {
- err = -ENOMEM;
- goto end_register;
- }
-
- range->start = addr;
- range->size = size;
-
- list_add_tail(&range->list, &io_range_list);
-
- spin_unlock(&io_range_lock);
-#endif
-
- return err;
-}
-
-phys_addr_t pci_pio_to_address(unsigned long pio)
-{
- phys_addr_t address = (phys_addr_t)OF_BAD_ADDR;
-
-#ifdef PCI_IOBASE
- struct io_range *range;
- resource_size_t allocated_size = 0;
-
- if (pio > IO_SPACE_LIMIT)
- return address;
-
- spin_lock(&io_range_lock);
- list_for_each_entry(range, &io_range_list, list) {
- if (pio >= allocated_size && pio < allocated_size + range->size) {
- address = range->start + pio - allocated_size;
- break;
- }
- allocated_size += range->size;
- }
- spin_unlock(&io_range_lock);
-#endif
-
- return address;
-}
-
-unsigned long __weak pci_address_to_pio(phys_addr_t address)
-{
-#ifdef PCI_IOBASE
- struct io_range *res;
- resource_size_t offset = 0;
- unsigned long addr = -1;
-
- spin_lock(&io_range_lock);
- list_for_each_entry(res, &io_range_list, list) {
- if (address >= res->start && address < res->start + res->size) {
- addr = address - res->start + offset;
- break;
- }
- offset += res->size;
- }
- spin_unlock(&io_range_lock);
-
- return addr;
-#else
- if (address > IO_SPACE_LIMIT)
- return (unsigned long)-1;
-
- return (unsigned long) address;
-#endif
-}
-
static int __of_address_to_resource(struct device_node *dev,
const __be32 *addrp, u64 size, unsigned int flags,
const char *name, struct resource *r)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 25e0327..bc0c914 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3021,6 +3021,121 @@ int pci_request_regions_exclusive(struct pci_dev *pdev, const char *res_name)
}
EXPORT_SYMBOL(pci_request_regions_exclusive);
+#ifdef PCI_IOBASE
+struct io_range {
+ struct list_head list;
+ phys_addr_t start;
+ resource_size_t size;
+};
+
+static LIST_HEAD(io_range_list);
+static DEFINE_SPINLOCK(io_range_lock);
+#endif
+
+/*
+ * Record the PCI IO range (expressed as CPU physical address + size).
+ * Return a negative value if an error has occured, zero otherwise
+ */
+int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
+{
+ int err = 0;
+
+#ifdef PCI_IOBASE

I understand that this moves code around, but those in-function
#ifdefs aren't nice. Any chance to get rid of them but putting whole
functions under the #ifdef?

Tomasz Nowicki

2016-05-11 07:37:14 UTC

I understand that this moves code around, but those in-function
#ifdefs aren't nice. Any chance to get rid of them but putting whole
functions under the #ifdef?

This is a __weak implementation, so assuming I would move #ifdef out of
function I need to provide another empty __weak stub. I do not know
which solution is more ugly. In any case we can do that cleanup separately.

Tomasz

Arnd Bergmann

2016-05-11 11:02:35 UTC

Post by Rafael J. Wysocki
I understand that this moves code around, but those in-function
#ifdefs aren't nice. Any chance to get rid of them but putting whole
functions under the #ifdef?

I'd vote for just dropping the __weak here, given that there is no
non-weak implementation. If we end up needing to override this for
some architecture or host bridge in the future, we can think about how
to best do that then.

I agree that should be a separate patch, this one should only move
code from one file to another.

Arnd

Tomasz Nowicki

2016-05-10 15:22:57 UTC

This patch provides a way to set the ACPI companion in PCI code.
We define acpi_pci_set_companion() to set the ACPI companion pointer and
call it from PCI core code. The function is stub for now.

Signed-off-by: Jayachandran C <***@broadcom.com>
Signed-off-by: Tomasz Nowicki <***@semihalf.com>
---
drivers/pci/probe.c | 2 ++
include/linux/pci-acpi.h | 4 ++++
2 files changed, 6 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8004f67..fb0b752 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -12,6 +12,7 @@
#include <linux/slab.h>
#include <linux/module.h>
#include <linux/cpumask.h>
+#include <linux/pci-acpi.h>
#include <linux/pci-aspm.h>
#include <linux/aer.h>
#include <linux/acpi.h>
@@ -2141,6 +2142,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
bridge->dev.parent = parent;
bridge->dev.release = pci_release_host_bridge_dev;
dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
+ acpi_pci_set_companion(bridge);
error = pcibios_root_bridge_prepare(bridge);
if (error) {
kfree(bridge);
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 09f9f02..1baa515 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -111,6 +111,10 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
#endif /* CONFIG_ACPI */

+static inline void acpi_pci_set_companion(struct pci_host_bridge *bridge)
+{
+}
+
static inline int acpi_pci_bus_domain_nr(struct pci_bus *bus)
{
return 0;

--
1.9.1

Rafael J. Wysocki

2016-05-10 18:37:14 UTC

Yes, we'll probably add something similar here.

Do I think now is the right time to do that? No.

Post by Tomasz Nowicki
error = pcibios_root_bridge_prepare(bridge);
if (error) {
kfree(bridge);
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 09f9f02..1baa515 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -111,6 +111,10 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
#endif /* CONFIG_ACPI */
+static inline void acpi_pci_set_companion(struct pci_host_bridge *bridge)
+{
+}
+
static inline int acpi_pci_bus_domain_nr(struct pci_bus *bus)
{
return 0;
--

Honestly, to me it looks like this series is trying very hard to avoid
doing any PCI host bridge configuration stuff from arch/arm64/
although (a) that might be simpler and (b) it would allow us to
identify the code that's common between *all* architectures using ACPI
support for host bridge configuration and to move *that* to a common
place later. As done here it seems to be following the "ARM64 is
generic and the rest of the world is special" line which isn't really
helpful.

Rafael J. Wysocki

2016-05-10 18:43:51 UTC

Yes, we'll probably add something similar here.
Do I think now is the right time to do that? No.

Speaking of which, at least one of the reasons why the ACPI PCI host
bridge thing on x86 and ia64 went to the arch code was to avoid
explicit references to ACPI-specific data types and related #ifdeffery
in the generic PCI code and data structures. If you are going to add
those references now anyway, that reason is not relevant any more and
all of that can just be reworked to refer to ACPI explicitly.

Lorenzo Pieralisi

2016-05-11 10:11:03 UTC

Yes, we'll probably add something similar here.
Do I think now is the right time to do that? No.

I think patch [1-2] should be merged regardless (they may require minor
tweaks if we decide to move pci_acpi_scan_root() to arch/arm64 though,
for include files location). I guess you are referring to patch 8 in
your comments above, which boils down to deciding whether:

- pci_acpi_scan_root() (and unfortunately all the MCFG/ECAM handling that
goes with it) should live in arch/arm64 or drivers/acpi

acpi_pci_bus_domain_nr() is a bit more problematic since it is meant
to be called from PCI core code (ARM64 selects PCI_DOMAINS_GENERIC for
DT and same kernel has to work with OF and ACPI selected) and it is
arch specific (because what we have in bus->sysdata is arch specific,
waiting for the domain number to be embedded in struct pci_host_bridge).

Your point is fair, I am not sure that moving the pci_acpi_scan_root()
to arch/arm64 would make things much simpler though, it is just a matter
of deciding where that code has to live.

How do you want us to proceed ?

Thanks,
Lorenzo

Rafael J. Wysocki

2016-05-11 20:31:01 UTC

On Wed, May 11, 2016 at 12:11 PM, Lorenzo Pieralisi

Yes, we'll probably add something similar here.
Do I think now is the right time to do that? No.

I think patch [1-2] should be merged regardless (they may require minor
tweaks if we decide to move pci_acpi_scan_root() to arch/arm64 though,
for include files location). I guess you are referring to patch 8 in
- pci_acpi_scan_root() (and unfortunately all the MCFG/ECAM handling that
goes with it) should live in arch/arm64 or drivers/acpi

To be precise, everything under #ifdef CONFIG_ACPI_PCI_HOST_GENERIC or
equivalent is de facto ARM64-specific, because (as it stands in the
patch series) ARM64 is the only architecture that will select that
option. Unless you are aware of any more architectures planning to
use ACPI (and I'm not aware of any), it will stay the only
architecture selecting it in the foreseeable future.

Therefore you could replace CONFIG_ACPI_PCI_HOST_GENERIC with
CONFIG_ARM64 everywhere in that code which is why in my opinion the
code should live somewhere under arch/arm64/.

Going forward, it should be possible to identify common parts of the
PCI host bridge configuration code in arch/ and move it to
drivers/acpi/ or drivers/pci/, but I bet that won't be the entire code
this series puts under CONFIG_ACPI_PCI_HOST_GENERIC.

The above leads to a quite straightforward conclusion about the order
in which to do things: I'd add ACPI support for PCI host bridge on
ARM64 following what's been done on ia64 (as x86 is more quirky and
kludgy overall) as far as reasonably possible first and then think
about moving common stuff to a common place.

Post by Lorenzo Pieralisi
acpi_pci_bus_domain_nr() is a bit more problematic since it is meant
to be called from PCI core code (ARM64 selects PCI_DOMAINS_GENERIC for
DT and same kernel has to work with OF and ACPI selected) and it is
arch specific (because what we have in bus->sysdata is arch specific,
waiting for the domain number to be embedded in struct pci_host_bridge).
Your point is fair, I am not sure that moving the pci_acpi_scan_root()
to arch/arm64 would make things much simpler though, it is just a matter
of deciding where that code has to live.
How do you want us to proceed ?

Pretty much as stated above. :-)

Thanks,
Rafael

Bjorn Helgaas

2016-05-11 22:43:26 UTC

Post by Rafael J. Wysocki
On Wed, May 11, 2016 at 12:11 PM, Lorenzo Pieralisi

Yes, we'll probably add something similar here.
Do I think now is the right time to do that? No.

I think patch [1-2] should be merged regardless (they may require minor
tweaks if we decide to move pci_acpi_scan_root() to arch/arm64 though,
for include files location). I guess you are referring to patch 8 in
- pci_acpi_scan_root() (and unfortunately all the MCFG/ECAM handling that
goes with it) should live in arch/arm64 or drivers/acpi

To be precise, everything under #ifdef CONFIG_ACPI_PCI_HOST_GENERIC or
equivalent is de facto ARM64-specific, because (as it stands in the
patch series) ARM64 is the only architecture that will select that
option. Unless you are aware of any more architectures planning to
use ACPI (and I'm not aware of any), it will stay the only
architecture selecting it in the foreseeable future.
Therefore you could replace CONFIG_ACPI_PCI_HOST_GENERIC with
CONFIG_ARM64 everywhere in that code which is why in my opinion the
code should live somewhere under arch/arm64/.
Going forward, it should be possible to identify common parts of the
PCI host bridge configuration code in arch/ and move it to
drivers/acpi/ or drivers/pci/, but I bet that won't be the entire code
this series puts under CONFIG_ACPI_PCI_HOST_GENERIC.
The above leads to a quite straightforward conclusion about the order
in which to do things: I'd add ACPI support for PCI host bridge on
ARM64 following what's been done on ia64 (as x86 is more quirky and
kludgy overall) as far as reasonably possible first and then think
about moving common stuff to a common place.

That does seem like a reasonable approach. I had hoped to get more of
this in for v4.7, but we don't have much time left. Maybe some of
Rafael's comments can be addressed by moving and slight restructuring
and we can still squeeze it in.

The first three patches:

PCI: Provide common functions for ECAM mapping
PCI: generic, thunder: Use generic ECAM API
PCI, of: Move PCI I/O space management to PCI core code

seem relatively straightforward, and I applied them to pci/arm64 with
the intent of merging them unless there are objections. I made the
following tweaks, mainly to try to improve some error messages:

diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
index 3d52005..e1add01 100644
--- a/drivers/pci/ecam.c
+++ b/drivers/pci/ecam.c
@@ -24,9 +24,9 @@
#include "ecam.h"

/*
- * On 64 bit systems, we do a single ioremap for the whole config space
- * since we have enough virtual address range available. On 32 bit, do an
- * ioremap per bus.
+ * On 64-bit systems, we do a single ioremap for the whole config space
+ * since we have enough virtual address range available. On 32-bit, we
+ * ioremap the config space for each bus individually.
*/
static const bool per_bus_mapping = !config_enabled(CONFIG_64BIT);

@@ -42,6 +42,7 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
{
struct pci_config_window *cfg;
unsigned int bus_range, bus_range_max, bsz;
+ struct resource *conflict;
int i, err;

if (busr->start > busr->end)
@@ -58,10 +59,10 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
bus_range = resource_size(&cfg->busr);
bus_range_max = resource_size(cfgres) >> ops->bus_shift;
if (bus_range > bus_range_max) {
- dev_warn(dev, "bus max %#x reduced to %#x",
- bus_range, bus_range_max);
bus_range = bus_range_max;
cfg->busr.end = busr->start + bus_range - 1;
+ dev_warn(dev, "ECAM area %pR can only accommodate %pR (reduced from %pR desired)\n",
+ cfgres, &cfg->busr, busr);
}
bsz = 1 << ops->bus_shift;

@@ -70,9 +71,11 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
cfg->res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
cfg->res.name = "PCI ECAM";

- err = request_resource(&iomem_resource, &cfg->res);
- if (err) {
- dev_err(dev, "request ECAM res %pR failed\n", &cfg->res);
+ conflict = request_resource(&iomem_resource, &cfg->res);
+ if (conflict) {
+ err = -EBUSY;
+ dev_err(dev, "can't claim ECAM area %pR: address conflict with %s %pR\n",
+ &cfg->res, conflict->name, conflict);
goto err_exit;
}

diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h
index 1ad2176..9878beb 100644
--- a/drivers/pci/ecam.h
+++ b/drivers/pci/ecam.h
@@ -33,7 +33,7 @@ struct pci_ecam_ops {

/*
* struct to hold the mappings of a config space window. This
- * is expected to be used as sysdata for PCI controlllers which
+ * is expected to be used as sysdata for PCI controllers that
* use ECAM.
*/
struct pci_config_window {
@@ -43,11 +43,11 @@ struct pci_config_window {
struct pci_ecam_ops *ops;
union {
void __iomem *win; /* 64-bit single mapping */
- void __iomem **winp; /* 32-bit per bus mapping */
+ void __iomem **winp; /* 32-bit per-bus mapping */
};
};

-/* create and free for pci_config_window */
+/* create and free pci_config_window */
struct pci_config_window *pci_ecam_create(struct device *dev,
struct resource *cfgres, struct resource *busr,
struct pci_ecam_ops *ops);
@@ -56,11 +56,11 @@ void pci_ecam_free(struct pci_config_window *cfg);
/* map_bus when ->sysdata is an instance of pci_config_window */
void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
int where);
-/* default ECAM ops, bus shift 20, generic read and write */
+/* default ECAM ops */
extern struct pci_ecam_ops pci_generic_ecam_ops;

#ifdef CONFIG_PCI_HOST_GENERIC
-/* for DT based pci controllers that support ECAM */
+/* for DT-based PCI controllers that support ECAM */
int pci_host_common_probe(struct platform_device *pdev,
struct pci_ecam_ops *ops);
#endif

Lorenzo Pieralisi

2016-05-12 10:01:06 UTC

Post by Bjorn Helgaas

Post by Rafael J. Wysocki
On Wed, May 11, 2016 at 12:11 PM, Lorenzo Pieralisi

Yes, we'll probably add something similar here.
Do I think now is the right time to do that? No.

I think patch [1-2] should be merged regardless (they may require minor
tweaks if we decide to move pci_acpi_scan_root() to arch/arm64 though,
for include files location). I guess you are referring to patch 8 in
- pci_acpi_scan_root() (and unfortunately all the MCFG/ECAM handling that
goes with it) should live in arch/arm64 or drivers/acpi

To be precise, everything under #ifdef CONFIG_ACPI_PCI_HOST_GENERIC or
equivalent is de facto ARM64-specific, because (as it stands in the
patch series) ARM64 is the only architecture that will select that
option. Unless you are aware of any more architectures planning to
use ACPI (and I'm not aware of any), it will stay the only
architecture selecting it in the foreseeable future.
Therefore you could replace CONFIG_ACPI_PCI_HOST_GENERIC with
CONFIG_ARM64 everywhere in that code which is why in my opinion the
code should live somewhere under arch/arm64/.
Going forward, it should be possible to identify common parts of the
PCI host bridge configuration code in arch/ and move it to
drivers/acpi/ or drivers/pci/, but I bet that won't be the entire code
this series puts under CONFIG_ACPI_PCI_HOST_GENERIC.
The above leads to a quite straightforward conclusion about the order
in which to do things: I'd add ACPI support for PCI host bridge on
ARM64 following what's been done on ia64 (as x86 is more quirky and
kludgy overall) as far as reasonably possible first and then think
about moving common stuff to a common place.

Yes, it seems like a reasonable approach, as long as we accept that
part of this series has to live in arch/arm64 otherwise we are going
round in circles (because that's the gist of this discussion, to
decide where this code has to live, I do not think there is any objection
to the code per-se anymore).

I suggest we post a v8 (with code move to arch/arm64) end of merge
window (or you prefer seeing patches now to prevent any additional
changes later ?), my aim is to get this into -next (whether via arm64 or
pci tree it has to be decided) as early as possible for next cycle (-rc1)
so that it can get exposure and testing, I do not think that missing the
merge window is a big issue if we agree that the code is ready to go.

Post by Bjorn Helgaas
PCI: Provide common functions for ECAM mapping
PCI: generic, thunder: Use generic ECAM API
PCI, of: Move PCI I/O space management to PCI core code
seem relatively straightforward, and I applied them to pci/arm64 with
the intent of merging them unless there are objections. I made the

Ok, thanks a lot !

Lorenzo

Post by Bjorn Helgaas
diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
index 3d52005..e1add01 100644
--- a/drivers/pci/ecam.c
+++ b/drivers/pci/ecam.c
@@ -24,9 +24,9 @@
#include "ecam.h"
/*
- * On 64 bit systems, we do a single ioremap for the whole config space
- * since we have enough virtual address range available. On 32 bit, do an
- * ioremap per bus.
+ * On 64-bit systems, we do a single ioremap for the whole config space
+ * since we have enough virtual address range available. On 32-bit, we
+ * ioremap the config space for each bus individually.
*/
static const bool per_bus_mapping = !config_enabled(CONFIG_64BIT);
@@ -42,6 +42,7 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
{
struct pci_config_window *cfg;
unsigned int bus_range, bus_range_max, bsz;
+ struct resource *conflict;
int i, err;
if (busr->start > busr->end)
@@ -58,10 +59,10 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
bus_range = resource_size(&cfg->busr);
bus_range_max = resource_size(cfgres) >> ops->bus_shift;
if (bus_range > bus_range_max) {
- dev_warn(dev, "bus max %#x reduced to %#x",
- bus_range, bus_range_max);
bus_range = bus_range_max;
cfg->busr.end = busr->start + bus_range - 1;
+ dev_warn(dev, "ECAM area %pR can only accommodate %pR (reduced from %pR desired)\n",
+ cfgres, &cfg->busr, busr);
}
bsz = 1 << ops->bus_shift;
@@ -70,9 +71,11 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
cfg->res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
cfg->res.name = "PCI ECAM";
- err = request_resource(&iomem_resource, &cfg->res);
- if (err) {
- dev_err(dev, "request ECAM res %pR failed\n", &cfg->res);
+ conflict = request_resource(&iomem_resource, &cfg->res);
+ if (conflict) {
+ err = -EBUSY;
+ dev_err(dev, "can't claim ECAM area %pR: address conflict with %s %pR\n",
+ &cfg->res, conflict->name, conflict);
goto err_exit;
}
diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h
index 1ad2176..9878beb 100644
--- a/drivers/pci/ecam.h
+++ b/drivers/pci/ecam.h
@@ -33,7 +33,7 @@ struct pci_ecam_ops {
/*
* struct to hold the mappings of a config space window. This
- * is expected to be used as sysdata for PCI controlllers which
+ * is expected to be used as sysdata for PCI controllers that
* use ECAM.
*/
struct pci_config_window {
@@ -43,11 +43,11 @@ struct pci_config_window {
struct pci_ecam_ops *ops;
union {
void __iomem *win; /* 64-bit single mapping */
- void __iomem **winp; /* 32-bit per bus mapping */
+ void __iomem **winp; /* 32-bit per-bus mapping */
};
};
-/* create and free for pci_config_window */
+/* create and free pci_config_window */
struct pci_config_window *pci_ecam_create(struct device *dev,
struct resource *cfgres, struct resource *busr,
struct pci_ecam_ops *ops);
@@ -56,11 +56,11 @@ void pci_ecam_free(struct pci_config_window *cfg);
/* map_bus when ->sysdata is an instance of pci_config_window */
void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
int where);
-/* default ECAM ops, bus shift 20, generic read and write */
+/* default ECAM ops */
extern struct pci_ecam_ops pci_generic_ecam_ops;
#ifdef CONFIG_PCI_HOST_GENERIC
-/* for DT based pci controllers that support ECAM */
+/* for DT-based PCI controllers that support ECAM */
int pci_host_common_probe(struct platform_device *pdev,
struct pci_ecam_ops *ops);
#endif

Jayachandran C

2016-05-12 10:44:37 UTC

Post by Bjorn Helgaas

Post by Rafael J. Wysocki
On Wed, May 11, 2016 at 12:11 PM, Lorenzo Pieralisi

Yes, we'll probably add something similar here.
Do I think now is the right time to do that? No.

I think patch [1-2] should be merged regardless (they may require minor
tweaks if we decide to move pci_acpi_scan_root() to arch/arm64 though,
for include files location). I guess you are referring to patch 8 in
- pci_acpi_scan_root() (and unfortunately all the MCFG/ECAM handling that
goes with it) should live in arch/arm64 or drivers/acpi

To be precise, everything under #ifdef CONFIG_ACPI_PCI_HOST_GENERIC or
equivalent is de facto ARM64-specific, because (as it stands in the
patch series) ARM64 is the only architecture that will select that
option. Unless you are aware of any more architectures planning to
use ACPI (and I'm not aware of any), it will stay the only
architecture selecting it in the foreseeable future.
Therefore you could replace CONFIG_ACPI_PCI_HOST_GENERIC with
CONFIG_ARM64 everywhere in that code which is why in my opinion the
code should live somewhere under arch/arm64/.
Going forward, it should be possible to identify common parts of the
PCI host bridge configuration code in arch/ and move it to
drivers/acpi/ or drivers/pci/, but I bet that won't be the entire code
this series puts under CONFIG_ACPI_PCI_HOST_GENERIC.
The above leads to a quite straightforward conclusion about the order
in which to do things: I'd add ACPI support for PCI host bridge on
ARM64 following what's been done on ia64 (as x86 is more quirky and
kludgy overall) as far as reasonably possible first and then think
about moving common stuff to a common place.

That does seem like a reasonable approach. I had hoped to get more of
this in for v4.7, but we don't have much time left. Maybe some of
Rafael's comments can be addressed by moving and slight restructuring
and we can still squeeze it in.
PCI: Provide common functions for ECAM mapping
PCI: generic, thunder: Use generic ECAM API
PCI, of: Move PCI I/O space management to PCI core code
seem relatively straightforward, and I applied them to pci/arm64 with
the intent of merging them unless there are objections. I made the
diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
index 3d52005..e1add01 100644
--- a/drivers/pci/ecam.c
+++ b/drivers/pci/ecam.c
@@ -24,9 +24,9 @@
#include "ecam.h"
/*
- * On 64 bit systems, we do a single ioremap for the whole config space
- * since we have enough virtual address range available. On 32 bit, do an
- * ioremap per bus.
+ * On 64-bit systems, we do a single ioremap for the whole config space
+ * since we have enough virtual address range available. On 32-bit, we
+ * ioremap the config space for each bus individually.
*/
static const bool per_bus_mapping = !config_enabled(CONFIG_64BIT);
@@ -42,6 +42,7 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
{
struct pci_config_window *cfg;
unsigned int bus_range, bus_range_max, bsz;
+ struct resource *conflict;
int i, err;
if (busr->start > busr->end)
@@ -58,10 +59,10 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
bus_range = resource_size(&cfg->busr);
bus_range_max = resource_size(cfgres) >> ops->bus_shift;
if (bus_range > bus_range_max) {
- dev_warn(dev, "bus max %#x reduced to %#x",
- bus_range, bus_range_max);
bus_range = bus_range_max;
cfg->busr.end = busr->start + bus_range - 1;
+ dev_warn(dev, "ECAM area %pR can only accommodate %pR (reduced from %pR desired)\n",
+ cfgres, &cfg->busr, busr);
}
bsz = 1 << ops->bus_shift;
@@ -70,9 +71,11 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
cfg->res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
cfg->res.name = "PCI ECAM";
- err = request_resource(&iomem_resource, &cfg->res);
- if (err) {
- dev_err(dev, "request ECAM res %pR failed\n", &cfg->res);
+ conflict = request_resource(&iomem_resource, &cfg->res);
+ if (conflict) {
+ err = -EBUSY;
+ dev_err(dev, "can't claim ECAM area %pR: address conflict with %s %pR\n",
+ &cfg->res, conflict->name, conflict);
goto err_exit;
}
diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h
index 1ad2176..9878beb 100644
--- a/drivers/pci/ecam.h
+++ b/drivers/pci/ecam.h
@@ -33,7 +33,7 @@ struct pci_ecam_ops {
/*
* struct to hold the mappings of a config space window. This
- * is expected to be used as sysdata for PCI controlllers which
+ * is expected to be used as sysdata for PCI controllers that
* use ECAM.
*/
struct pci_config_window {
@@ -43,11 +43,11 @@ struct pci_config_window {
struct pci_ecam_ops *ops;
union {
void __iomem *win; /* 64-bit single mapping */
- void __iomem **winp; /* 32-bit per bus mapping */
+ void __iomem **winp; /* 32-bit per-bus mapping */
};
};
-/* create and free for pci_config_window */
+/* create and free pci_config_window */
struct pci_config_window *pci_ecam_create(struct device *dev,
struct resource *cfgres, struct resource *busr,
struct pci_ecam_ops *ops);
@@ -56,11 +56,11 @@ void pci_ecam_free(struct pci_config_window *cfg);
/* map_bus when ->sysdata is an instance of pci_config_window */
void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
int where);
-/* default ECAM ops, bus shift 20, generic read and write */
+/* default ECAM ops */
extern struct pci_ecam_ops pci_generic_ecam_ops;
#ifdef CONFIG_PCI_HOST_GENERIC
-/* for DT based pci controllers that support ECAM */
+/* for DT-based PCI controllers that support ECAM */
int pci_host_common_probe(struct platform_device *pdev,
struct pci_ecam_ops *ops);
#endif

If we are moving the ACPI/PCI code from drivers/acpi to
arch/arm64/ , there is an issue in having the header file
ecam.h in drivers/pci

The current include of "../pci/ecam.h" is slightly ugly (Arnd
and David had already noted this), but including the driver
header from arch code would be even worse.

I can either merge ecam.h into include/linux/pci.h
or move it to a new file include/linux/pci-ecam.h, any
suggestion on which is preferable?

JC.

Rafael J. Wysocki

2016-05-12 11:27:40 UTC

Post by Rafael J. Wysocki
On Wed, May 11, 2016 at 12:11 PM, Lorenzo Pieralisi

[cut]

Post by Jayachandran C
If we are moving the ACPI/PCI code from drivers/acpi to
arch/arm64/ , there is an issue in having the header file
ecam.h in drivers/pci
The current include of "../pci/ecam.h" is slightly ugly (Arnd
and David had already noted this), but including the driver
header from arch code would be even worse.
I can either merge ecam.h into include/linux/pci.h
or move it to a new file include/linux/pci-ecam.h, any
suggestion on which is preferable?

My preference would be pci-ecam.h as we did a similar thing for
pci-dma.h, for example, but basically this is up to Bjorn.

Lorenzo Pieralisi

2016-05-13 10:33:06 UTC

Post by Rafael J. Wysocki
On Wed, May 11, 2016 at 12:11 PM, Lorenzo Pieralisi

[cut]

My preference would be pci-ecam.h as we did a similar thing for
pci-dma.h, for example, but basically this is up to Bjorn.

A word of caution for all interested parties, what we may move
to arch/arm64 (if Catalin and Will are ok with that) here is content
of drivers/acpi/pci_root_generic.c, not drivers/acpi/pci_mcfg.c (and
definitely not the MCFG quirks handling that is coming up next on top
of this series).

I just wanted to make sure we understand that MCFG quirks handling
like eg:

https://lkml.org/lkml/2016/4/28/790

that is coming up following this series has no chance whatsoever to
be handled within arch/arm64, it is just not going to happen.

Maybe I am jumping the gun, I just want to make sure that everyone is
aware that moving part of this series to arch/arm64 has implications,
(and that's why I said that moving part of this code to arch/arm64 is
not as simple as it looks) it may be ok to have an ACPI PCI
implementation that is arch/arm64 specific (mostly for IO space and PCI
resources assignment handling that unfortunately is not uniform across
X86, IA64 and ARM64), but MCFG quirks and related platform code stay out
of arch/arm64 I guess we are all aware of that, just wanted to make
sure :)

Lorenzo

Tomasz Nowicki

2016-05-12 10:50:22 UTC

Post by Bjorn Helgaas

Post by Rafael J. Wysocki
On Wed, May 11, 2016 at 12:11 PM, Lorenzo Pieralisi

Yes, we'll probably add something similar here.
Do I think now is the right time to do that? No.

I think patch [1-2] should be merged regardless (they may require minor
tweaks if we decide to move pci_acpi_scan_root() to arch/arm64 though,
for include files location). I guess you are referring to patch 8 in
- pci_acpi_scan_root() (and unfortunately all the MCFG/ECAM handling that
goes with it) should live in arch/arm64 or drivers/acpi

To be precise, everything under #ifdef CONFIG_ACPI_PCI_HOST_GENERIC or
equivalent is de facto ARM64-specific, because (as it stands in the
patch series) ARM64 is the only architecture that will select that
option. Unless you are aware of any more architectures planning to
use ACPI (and I'm not aware of any), it will stay the only
architecture selecting it in the foreseeable future.
Therefore you could replace CONFIG_ACPI_PCI_HOST_GENERIC with
CONFIG_ARM64 everywhere in that code which is why in my opinion the
code should live somewhere under arch/arm64/.
Going forward, it should be possible to identify common parts of the
PCI host bridge configuration code in arch/ and move it to
drivers/acpi/ or drivers/pci/, but I bet that won't be the entire code
this series puts under CONFIG_ACPI_PCI_HOST_GENERIC.
The above leads to a quite straightforward conclusion about the order
in which to do things: I'd add ACPI support for PCI host bridge on
ARM64 following what's been done on ia64 (as x86 is more quirky and
kludgy overall) as far as reasonably possible first and then think
about moving common stuff to a common place.

We need request_resource_conflict here then:
- conflict = request_resource(&iomem_resource, &cfg->res);
+ conflict = request_resource_conflict(&iomem_resource, &cfg->res);

Thanks,
Tomasz

Bjorn Helgaas

2016-05-12 12:08:47 UTC

Post by Bjorn Helgaas
@@ -70,9 +71,11 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
cfg->res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
cfg->res.name = "PCI ECAM";
- err = request_resource(&iomem_resource, &cfg->res);
- if (err) {
- dev_err(dev, "request ECAM res %pR failed\n", &cfg->res);
+ conflict = request_resource(&iomem_resource, &cfg->res);

- conflict = request_resource(&iomem_resource, &cfg->res);
+ conflict = request_resource_conflict(&iomem_resource, &cfg->res);

Whoops, fixed, thanks!

Dongdong Liu

2016-05-17 03:14:43 UTC

Hi Tomasz

I used the patchset and added "PATCH V6 11/13 specic quirks", tested on HiSilicon D02 board but met the below problem.

[ 2.614115] [<ffffff80083b13bc>] hisi_pcie_init+0x6c/0x1ec
[ 2.619571] [<ffffff80083ab060>] pci_ecam_create+0x130/0x1ec
[ 2.625209] [<ffffff80083f3764>] pci_acpi_scan_root+0x160/0x218
[ 2.631096] [<ffffff80083d1f6c>] acpi_pci_root_add+0x36c/0x42c
[ 2.636897] [<ffffff80083ce36c>] acpi_bus_attach+0xe4/0x1a8
[ 2.642438] [<ffffff80083ce3d8>] acpi_bus_attach+0x150/0x1a8
[ 2.648066] [<ffffff80083ce3d8>] acpi_bus_attach+0x150/0x1a8
[ 2.653693] [<ffffff80083ce55c>] acpi_bus_scan+0x64/0x74
[ 2.658975] [<ffffff8008ae665c>] acpi_scan_init+0x5c/0x19c
[ 2.664429] [<ffffff8008ae6408>] acpi_init+0x280/0x2a4
[ 2.669538] [<ffffff80080829e8>] do_one_initcall+0x8c/0x19c
[ 2.675080] [<ffffff8008ac3af8>] kernel_init_freeable+0x14c/0x1ec
[ 2.681139] [<ffffff80087a8438>] kernel_init+0x10/0xfc
[ 2.686248] [<ffffff8008085e10>] ret_from_fork+0x10/0x40

In hisi_pcie_init, I used "struct acpi_device *device = ACPI_COMPANION(dev);".
I found the reason is V7 lack the below code. I added the below code, it worked ok.

[PATCH V6 01/13] pci, acpi, x86, ia64: Move ACPI host bridge device companion assignment to core code.
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -564,6 +564,11 @@ static int acpi_pci_root_add(struct acpi_device *device,
}
}

+ /*
+ * pci_create_root_bus() needs to detect the parent device type,
+ * so initialize its companion data accordingly.
+ */
+ ACPI_COMPANION_SET(&device->dev, device);

This code will be upstreamed with the "PATCH V6 11/13 specic quirks" in next time after the patchset is accepted.
Right ?

Post by Tomasz Nowicki
This patch provides a way to set the ACPI companion in PCI code.
We define acpi_pci_set_companion() to set the ACPI companion pointer and
call it from PCI core code. The function is stub for now.
---
drivers/pci/probe.c | 2 ++
include/linux/pci-acpi.h | 4 ++++
2 files changed, 6 insertions(+)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8004f67..fb0b752 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -12,6 +12,7 @@
#include <linux/slab.h>
#include <linux/module.h>
#include <linux/cpumask.h>
+#include <linux/pci-acpi.h>
#include <linux/pci-aspm.h>
#include <linux/aer.h>
#include <linux/acpi.h>
@@ -2141,6 +2142,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
bridge->dev.parent = parent;
bridge->dev.release = pci_release_host_bridge_dev;
dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
+ acpi_pci_set_companion(bridge);
error = pcibios_root_bridge_prepare(bridge);
if (error) {
kfree(bridge);
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 09f9f02..1baa515 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -111,6 +111,10 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
#endif /* CONFIG_ACPI */
+static inline void acpi_pci_set_companion(struct pci_host_bridge *bridge)
+{
+}
+
static inline int acpi_pci_bus_domain_nr(struct pci_bus *bus)
{
return 0;

Tomasz Nowicki

2016-05-17 13:44:59 UTC

Post by Gabriele Paoloni
Hi Tomasz
I used the patchset and added "PATCH V6 11/13 specic quirks", tested on
HiSilicon D02 board but met the below problem.
[ 2.614115] [<ffffff80083b13bc>] hisi_pcie_init+0x6c/0x1ec
[ 2.619571] [<ffffff80083ab060>] pci_ecam_create+0x130/0x1ec
[ 2.625209] [<ffffff80083f3764>] pci_acpi_scan_root+0x160/0x218
[ 2.631096] [<ffffff80083d1f6c>] acpi_pci_root_add+0x36c/0x42c
[ 2.636897] [<ffffff80083ce36c>] acpi_bus_attach+0xe4/0x1a8
[ 2.642438] [<ffffff80083ce3d8>] acpi_bus_attach+0x150/0x1a8
[ 2.648066] [<ffffff80083ce3d8>] acpi_bus_attach+0x150/0x1a8
[ 2.653693] [<ffffff80083ce55c>] acpi_bus_scan+0x64/0x74
[ 2.658975] [<ffffff8008ae665c>] acpi_scan_init+0x5c/0x19c
[ 2.664429] [<ffffff8008ae6408>] acpi_init+0x280/0x2a4
[ 2.669538] [<ffffff80080829e8>] do_one_initcall+0x8c/0x19c
[ 2.675080] [<ffffff8008ac3af8>] kernel_init_freeable+0x14c/0x1ec
[ 2.681139] [<ffffff80087a8438>] kernel_init+0x10/0xfc
[ 2.686248] [<ffffff8008085e10>] ret_from_fork+0x10/0x40
In hisi_pcie_init, I used "struct acpi_device *device =
ACPI_COMPANION(dev);".
I found the reason is V7 lack the below code. I added the below code, it worked ok.
[PATCH V6 01/13] pci, acpi, x86, ia64: Move ACPI host bridge device
companion assignment to core code.
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -564,6 +564,11 @@ static int acpi_pci_root_add(struct acpi_device *device,
}
}
+ /*
+ * pci_create_root_bus() needs to detect the parent device type,
+ * so initialize its companion data accordingly.
+ */
+ ACPI_COMPANION_SET(&device->dev, device);
This code will be upstreamed with the "PATCH V6 11/13 specic quirks" in
next time after the patchset is accepted.
Right ?

We had that patch in previous series to retrieve PCI domain nicely. But
that has bad implication to userspace. See:
https://lkml.org/lkml/2016/5/9/918

I understand that:
[PATCH V6 01/13] pci, acpi, x86, ia64: Move ACPI host bridge device
companion assignment to core code.
helps to get firmware specific info in hisi_pcie_init but we need to
figure out something better for quirk handling too.

Tomasz

Tomasz Nowicki

2016-05-10 15:23:15 UTC

It is very useful to release I/O resources so that the same I/O resources
can be allocated again (pci_remap_iospace), like in PCI hotplug removal
scenario. Therefore this patch implements new pci_unmap_iospace call which
unmaps I/O space as the symmetry to pci_remap_iospace.

Signed-off-by: Sinan Kaya <***@codeaurora.org>
Signed-off-by: Tomasz Nowicki <***@semihalf.com>
---
drivers/pci/pci.c | 24 ++++++++++++++++++++++++
include/linux/pci.h | 1 +
2 files changed, 25 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bc0c914..ff97a0b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -25,6 +25,7 @@
#include <linux/device.h>
#include <linux/pm_runtime.h>
#include <linux/pci_hotplug.h>
+#include <linux/vmalloc.h>
#include <asm/setup.h>
#include <linux/aer.h>
#include "pci.h"
@@ -3167,6 +3168,29 @@ int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
#endif
}

+/**
+ * pci_unmap_iospace - Unmap the memory mapped I/O space
+ * @res: resource to be unmapped
+ *
+ * Unmap the CPU virtual address @res from virtual address space.
+ * Only architectures that have memory mapped IO functions defined
+ * (and the PCI_IOBASE value defined) should call this function.
+ */
+void pci_unmap_iospace(struct resource *res)
+{
+#if defined(PCI_IOBASE) && defined(CONFIG_MMU)
+ unsigned long vaddr = (unsigned long)PCI_IOBASE + res->start;
+
+ unmap_kernel_range(vaddr, resource_size(res));
+#else
+ /*
+ * This architecture does not have memory mapped I/O space,
+ * so this function should never be called.
+ */
+ WARN_ONCE(1, "This architecture does not support memory mapped I/O\n");
+#endif
+}
+
static void __pci_set_master(struct pci_dev *dev, bool enable)
{
u16 old_cmd, cmd;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d2a57f3..d6ea6ce 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1169,6 +1169,7 @@ int pci_register_io_range(phys_addr_t addr, resource_size_t size);
unsigned long pci_address_to_pio(phys_addr_t addr);
phys_addr_t pci_pio_to_address(unsigned long pio);
int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr);
+void pci_unmap_iospace(struct resource *res);

static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
{

--
1.9.1

Jayachandran C

2016-05-23 08:29:24 UTC

Post by Tomasz Nowicki
It is very useful to release I/O resources so that the same I/O resources
can be allocated again (pci_remap_iospace), like in PCI hotplug removal
scenario. Therefore this patch implements new pci_unmap_iospace call which
unmaps I/O space as the symmetry to pci_remap_iospace.
---
drivers/pci/pci.c | 24 ++++++++++++++++++++++++
include/linux/pci.h | 1 +
2 files changed, 25 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bc0c914..ff97a0b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -25,6 +25,7 @@
#include <linux/device.h>
#include <linux/pm_runtime.h>
#include <linux/pci_hotplug.h>
+#include <linux/vmalloc.h>
#include <asm/setup.h>
#include <linux/aer.h>
#include "pci.h"
@@ -3167,6 +3168,29 @@ int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
#endif
}
+/**
+ * pci_unmap_iospace - Unmap the memory mapped I/O space
+ *
+ * Only architectures that have memory mapped IO functions defined
+ * (and the PCI_IOBASE value defined) should call this function.
+ */
+void pci_unmap_iospace(struct resource *res)
+{
+#if defined(PCI_IOBASE) && defined(CONFIG_MMU)
+ unsigned long vaddr = (unsigned long)PCI_IOBASE + res->start;
+
+ unmap_kernel_range(vaddr, resource_size(res));
+#else
+ /*
+ * This architecture does not have memory mapped I/O space,
+ * so this function should never be called.
+ */
+ WARN_ONCE(1, "This architecture does not support memory mapped I/O\n");
+#endif
+}

WARN is not needed here, since we would have already done it in
pci_remap_iospace.

Ideally, we should undo the pci_register_io_range as well, but
re-registering the same range seems to be fine.

JC.

Tomasz Nowicki

2016-05-10 15:23:44 UTC

From: Jayachandran C <***@broadcom.com>

Add config option PCI_ECAM and file drivers/pci/ecam.c to provide
generic functions for accessing memory mapped PCI config space.

The API is defined in drivers/pci/ecam.h and is written to replace the
API in drivers/pci/host/pci-host-common.h. The file defines a new
'struct pci_config_window' to hold the information related to a PCI
config area and its mapping. This structure is expected to be used as
sysdata for controllers that have ECAM based mapping.

Helper functions are provided to setup the mapping, free the mapping
and to implement the map_bus method in 'struct pci_ops'

Signed-off-by: Jayachandran C <***@broadcom.com>
---
drivers/pci/Kconfig | 3 +
drivers/pci/Makefile | 2 +
drivers/pci/ecam.c | 161 +++++++++++++++++++++++++++++++++++++++++++++++++++
drivers/pci/ecam.h | 62 ++++++++++++++++++++
4 files changed, 228 insertions(+)
create mode 100644 drivers/pci/ecam.c
create mode 100644 drivers/pci/ecam.h

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 209292e..56389be 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -83,6 +83,9 @@ config HT_IRQ
config PCI_ATS
bool

+config PCI_ECAM
+ bool
+
config PCI_IOV
bool "PCI IOV support"
depends on PCI
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 2154092..1fa6925 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -55,6 +55,8 @@ obj-$(CONFIG_PCI_SYSCALL) += syscall.o

obj-$(CONFIG_PCI_STUB) += pci-stub.o

+obj-$(CONFIG_PCI_ECAM) += ecam.o
+
obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o

obj-$(CONFIG_OF) += of.o
diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
new file mode 100644
index 0000000..3d52005
--- /dev/null
+++ b/drivers/pci/ecam.c
@@ -0,0 +1,161 @@
+/*
+ * Copyright 2016 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation (the "GPL").
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License version 2 (GPLv2) for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * version 2 (GPLv2) along with this source code.
+ */
+
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/slab.h>
+
+#include "ecam.h"
+
+/*
+ * On 64 bit systems, we do a single ioremap for the whole config space
+ * since we have enough virtual address range available. On 32 bit, do an
+ * ioremap per bus.
+ */
+static const bool per_bus_mapping = !config_enabled(CONFIG_64BIT);
+
+/*
+ * Create a PCI config space window
+ * - reserve mem region
+ * - alloc struct pci_config_window with space for all mappings
+ * - ioremap the config space
+ */
+struct pci_config_window *pci_ecam_create(struct device *dev,
+ struct resource *cfgres, struct resource *busr,
+ struct pci_ecam_ops *ops)
+{
+ struct pci_config_window *cfg;
+ unsigned int bus_range, bus_range_max, bsz;
+ int i, err;
+
+ if (busr->start > busr->end)
+ return ERR_PTR(-EINVAL);
+
+ cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
+ if (!cfg)
+ return ERR_PTR(-ENOMEM);
+
+ cfg->ops = ops;
+ cfg->busr.start = busr->start;
+ cfg->busr.end = busr->end;
+ cfg->busr.flags = IORESOURCE_BUS;
+ bus_range = resource_size(&cfg->busr);
+ bus_range_max = resource_size(cfgres) >> ops->bus_shift;
+ if (bus_range > bus_range_max) {
+ dev_warn(dev, "bus max %#x reduced to %#x",
+ bus_range, bus_range_max);
+ bus_range = bus_range_max;
+ cfg->busr.end = busr->start + bus_range - 1;
+ }
+ bsz = 1 << ops->bus_shift;
+
+ cfg->res.start = cfgres->start;
+ cfg->res.end = cfgres->end;
+ cfg->res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
+ cfg->res.name = "PCI ECAM";
+
+ err = request_resource(&iomem_resource, &cfg->res);
+ if (err) {
+ dev_err(dev, "request ECAM res %pR failed\n", &cfg->res);
+ goto err_exit;
+ }
+
+ if (per_bus_mapping) {
+ cfg->winp = kcalloc(bus_range, sizeof(*cfg->winp), GFP_KERNEL);
+ if (!cfg->winp)
+ goto err_exit_malloc;
+ for (i = 0; i < bus_range; i++) {
+ cfg->winp[i] = ioremap(cfgres->start + i * bsz, bsz);
+ if (!cfg->winp[i])
+ goto err_exit_iomap;
+ }
+ } else {
+ cfg->win = ioremap(cfgres->start, bus_range * bsz);
+ if (!cfg->win)
+ goto err_exit_iomap;
+ }
+
+ if (ops->init) {
+ err = ops->init(dev, cfg);
+ if (err)
+ goto err_exit;
+ }
+ dev_info(dev, "ECAM at %pR for %pR\n", &cfg->res, &cfg->busr);
+ return cfg;
+
+err_exit_iomap:
+ dev_err(dev, "ECAM ioremap failed\n");
+err_exit_malloc:
+ err = -ENOMEM;
+err_exit:
+ pci_ecam_free(cfg);
+ return ERR_PTR(err);
+}
+
+void pci_ecam_free(struct pci_config_window *cfg)
+{
+ int i;
+
+ if (per_bus_mapping) {
+ if (cfg->winp) {
+ for (i = 0; i < resource_size(&cfg->busr); i++)
+ if (cfg->winp[i])
+ iounmap(cfg->winp[i]);
+ kfree(cfg->winp);
+ }
+ } else {
+ if (cfg->win)
+ iounmap(cfg->win);
+ }
+ if (cfg->res.parent)
+ release_resource(&cfg->res);
+ kfree(cfg);
+}
+
+/*
+ * Function to implement the pci_ops ->map_bus method
+ */
+void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
+ int where)
+{
+ struct pci_config_window *cfg = bus->sysdata;
+ unsigned int devfn_shift = cfg->ops->bus_shift - 8;
+ unsigned int busn = bus->number;
+ void __iomem *base;
+
+ if (busn < cfg->busr.start || busn > cfg->busr.end)
+ return NULL;
+
+ busn -= cfg->busr.start;
+ if (per_bus_mapping)
+ base = cfg->winp[busn];
+ else
+ base = cfg->win + (busn << cfg->ops->bus_shift);
+ return base + (devfn << devfn_shift) + where;
+}
+
+/* ECAM ops */
+struct pci_ecam_ops pci_generic_ecam_ops = {
+ .bus_shift = 20,
+ .pci_ops = {
+ .map_bus = pci_ecam_map_bus,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,
+ }
+};
diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h
new file mode 100644
index 0000000..57de00d
--- /dev/null
+++ b/drivers/pci/ecam.h
@@ -0,0 +1,62 @@
+/*
+ * Copyright 2016 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation (the "GPL").
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License version 2 (GPLv2) for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * version 2 (GPLv2) along with this source code.
+ */
+#ifndef DRIVERS_PCI_ECAM_H
+#define DRIVERS_PCI_ECAM_H
+
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+
+/*
+ * struct to hold pci ops and bus shift of the config window
+ * for a PCI controller.
+ */
+struct pci_config_window;
+struct pci_ecam_ops {
+ unsigned int bus_shift;
+ struct pci_ops pci_ops;
+ int (*init)(struct device *,
+ struct pci_config_window *);
+};
+
+/*
+ * struct to hold the mappings of a config space window. This
+ * is expected to be used as sysdata for PCI controlllers which
+ * use ECAM.
+ */
+struct pci_config_window {
+ struct resource res;
+ struct resource busr;
+ void *priv;
+ struct pci_ecam_ops *ops;
+ union {
+ void __iomem *win; /* 64-bit single mapping */
+ void __iomem **winp; /* 32-bit per bus mapping */
+ };
+};
+
+/* create and free for pci_config_window */
+struct pci_config_window *pci_ecam_create(struct device *dev,
+ struct resource *cfgres, struct resource *busr,
+ struct pci_ecam_ops *ops);
+void pci_ecam_free(struct pci_config_window *cfg);
+
+/* map_bus when ->sysdata is an instance of pci_config_window */
+void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
+ int where);
+/* default ECAM ops, bus shift 20, generic read and write */
+extern struct pci_ecam_ops pci_generic_ecam_ops;
+
+#endif

--
1.9.1

Gabriele Paoloni

2016-05-11 10:42:44 UTC

Hi Tomasz

-----Original Message-----
Sent: 10 May 2016 16:20
Subject: [PATCH V7 00/11] Support for generic ACPI based PCI host
controller
From the functionality point of view this series may be split into the
1. New ECAM API and update for users of the pci-host-common API
2. Necessary fixes as the preparation for using driver on ARM64.
3. Use new MCFG interface and implement generic ACPI based PCI host controller driver.
4. Enable above driver on ARM64
This has been tested on Cavium ThunderX server. Any help in reviewing and
testing is very appreciated.
v6 -> v7
- drop quirks handling

Maybe I missed something in the v6 discussion thread; when was it
decided to drop quirk handling?

I think it is important to have this in place to accommodate different
vendors. If the intention is to keep this patchset "clean" maybe
we can add it as a separate patch on top later on...

What’s your view?

Thanks

Gab

- changes for ACPI companion and domain number assignment approach
- implement arch pcibios_{add|remove}_bus and call
acpi_pci_{add|remove}_bus from there
- cleanups around nomenclature
- use resources oriented API for ECAM
- fix for based address calculation before mapping ECAM region
- remove useless lock for MCFG lookup
- move MCFG stuff to separated file pci_mcfg.c
- drop MCFG entries caching
- rebase against 4.6-rc7
v5 -> v6
- drop idea of x86 MMCONFIG code refactoring
https://lkml.org/lkml/2016/4/11/907
git: https://github.com/jchandra-brcm/linux/ (arm64-acpi-pci-v3)
- integrate Sinan's fix for releasing IO resources, see patch [06/13]
- added ACPI support for ThunderX ECAM and PEM drivers
- rebase against 4.6-rc2
v4 -> v5
- drop MCFG refactoring group patches 1-6 from series v4 and integrate Jayachandran's patch
https://patchwork.ozlabs.org/patch/575525/
- rewrite PCI legacy IRQs allocation
- squash two patches 11 and 12 from series v4, fixed bisection issue
- changelog improvements
- rebase against 4.5-rc3
v3 -> v4
- drop Jiang's fix
http://lkml.iu.edu/hypermail/linux/kernel/1601.1/04318.html
- add Lorenzo's fix patch 19/24
- ACPI PCI bus domain number assigning cleanup
- change resource management, we now claim and reassign resources
- improvements for applying quirks
- drop Matthew's http://www.spinics.net/lists/linux-pci/msg45950.html dependency
- rebase against 4.5-rc1
v2 -> v3
- fix legacy IRQ assigning and IO ports registration
- remove reference to arch specific companion device for ia64
- move ACPI PCI host controller driver to pci_root.c
- drop generic domain assignment for x86 and ia64 as I am not
able to run all necessary test variants
- drop patch which cleaned legacy IRQ assignment since it belongs to
https://patchwork.ozlabs.org/patch/557504/
- extend MCFG quirk code
- rebase against 4.4
v1 -> v2
- move non-arch specific piece of code to dirver/acpi/ directory
- fix IO resource handling
- introduce PCI config accessors quirks matching
- moved ACPI_COMPANION_SET to generic code
v1 - https://lkml.org/lkml/2015/10/27/504
v2 - https://lkml.org/lkml/2015/12/16/246
v3 - http://lkml.iu.edu/hypermail/linux/kernel/1601.1/04308.html
v4 - https://lkml.org/lkml/2016/2/4/646
v5 - https://lkml.org/lkml/2016/2/16/426
v6 - https://lkml.org/lkml/2016/4/15/594
PCI: Provide common functions for ECAM mapping
PCI: generic, thunder: update to use generic ECAM API
pci, of: Move the PCI I/O space management to PCI core code.
pci: Add new function to unmap IO resources.
acpi, pci: Support IO resources when parsing PCI host bridge
resources.
pci, acpi: Provide a way to assign bus domain number.
pci, acpi: Handle ACPI companion assignment.
pci, acpi: Support for ACPI based generic PCI host controller
arm64, pci, acpi: ACPI support for legacy IRQs parsing and
consolidation with DT code.
arm64, pci, acpi: Provide ACPI-specific prerequisites for PCI bus
enumeration.
arm64, pci, acpi: Start using ACPI based PCI host controller driver
for ARM64.
arch/arm64/Kconfig | 1 +
arch/arm64/kernel/pci.c | 34 +++-----
drivers/acpi/Kconfig | 8 ++
drivers/acpi/Makefile | 1 +
drivers/acpi/pci_mcfg.c | 97 ++++++++++++++++++++++
drivers/acpi/pci_root.c | 33 ++++++++
drivers/acpi/pci_root_generic.c | 149
+++++++++++++++++++++++++++++++++
drivers/of/address.c | 116 +-------------------------
drivers/pci/Kconfig | 3 +
drivers/pci/Makefile | 2 +
drivers/pci/ecam.c | 161
++++++++++++++++++++++++++++++++++++
drivers/pci/ecam.h | 72 ++++++++++++++++
drivers/pci/host/Kconfig | 1 +
drivers/pci/host/pci-host-common.c | 114 +++++++++++--------------
drivers/pci/host/pci-host-common.h | 47 -----------
drivers/pci/host/pci-host-generic.c | 52 +++---------
drivers/pci/host/pci-thunder-ecam.c | 39 ++-------
drivers/pci/host/pci-thunder-pem.c | 92 ++++++++++-----------
drivers/pci/pci.c | 150
++++++++++++++++++++++++++++++++-
drivers/pci/probe.c | 2 +
include/linux/of_address.h | 9 --
include/linux/pci-acpi.h | 14 ++++
include/linux/pci.h | 11 ++-
23 files changed, 823 insertions(+), 385 deletions(-)
create mode 100644 drivers/acpi/pci_mcfg.c
create mode 100644 drivers/acpi/pci_root_generic.c
create mode 100644 drivers/pci/ecam.c
create mode 100644 drivers/pci/ecam.h
delete mode 100644 drivers/pci/host/pci-host-common.h
--
1.9.1

Tomasz Nowicki

2016-05-11 11:08:15 UTC

Hi Gabriele,

Post by Gabriele Paoloni
Hi Tomasz

Maybe I missed something in the v6 discussion thread; when was it
decided to drop quirk handling?

I had such requests in previous series.

Post by Gabriele Paoloni
I think it is important to have this in place to accommodate different
vendors. If the intention is to keep this patchset "clean" maybe
we can add it as a separate patch on top later on...
What’s your view?

Yes, keeping these things separated should help in review. Obviously I
agree that we need quirk handling but currently there is no
implementation which we all agree upon. For the test, you can use quirk
handling approach from the previous series until we sort out final solution.

Thanks,
Tomasz

Gabriele Paoloni

2016-05-11 12:54:43 UTC

Hi Tomasz

-----Original Message-----
Sent: 11 May 2016 12:08
Subject: Re: [PATCH V7 00/11] Support for generic ACPI based PCI host
controller
Hi Gabriele,

Post by Gabriele Paoloni
Hi Tomasz

-----Original Message-----
Sent: 10 May 2016 16:20
Subject: [PATCH V7 00/11] Support for generic ACPI based PCI host
controller
From the functionality point of view this series may be split into

the

1. New ECAM API and update for users of the pci-host-common API
2. Necessary fixes as the preparation for using driver on ARM64.
3. Use new MCFG interface and implement generic ACPI based PCI host
controller driver.
4. Enable above driver on ARM64
This has been tested on Cavium ThunderX server. Any help in

reviewing

and
testing is very appreciated.
v6 -> v7
- drop quirks handling

Maybe I missed something in the v6 discussion thread; when was it
decided to drop quirk handling?

I had such requests in previous series.

Post by Gabriele Paoloni
I think it is important to have this in place to accommodate

different

Post by Gabriele Paoloni
vendors. If the intention is to keep this patchset "clean" maybe
we can add it as a separate patch on top later on...
What's your view?

Great

This explains perfectly

We will apply previous series quirks and try to test the patchset
from Huawei side

Many Thanks

Gab

Thanks,
Tomasz

Jon Masters

2016-05-20 04:41:51 UTC

Hi Tomasz, all,

Post by Tomasz Nowicki
v6 -> v7
- drop quirks handling

Maybe I missed something in the v6 discussion thread; when was it
decided to drop quirk handling?

I had such requests in previous series.

A quick note on quirk handling. This, I believe, applies post-merge of
the base infrastructure, which I realize will likely not have quirks.

We've some "gen1" ARMv8 server platforms where we end up doing quirks
(for things like forcing 32-bit config space accessors and the like) due
to people repurposing existing embedded PCIe IP blocks or using them for
the first time (especially in servers), and those being involved in the
design not necessarily seeing this problem ahead of time, or not
realizing that it would be an issue for servers. In the early days of
ARM server designs 3-4 years ago, many of us had never really played
with ECAM or realized how modern topologies are built.

Anyway. We missed this one in our SBSA requirements. They say (words to
the effect of) "thou shalt do PCIe the way it is done on servers" but
they aren't prescriptive, and they don't tell people how that actually
is in reality. That is being fixed. A lot of things are happening behind
the scenes - especially with third party IP block providers (all of whom
myself and others are speaking with directly about this) - to ensure
that the next wave of designs won't repeat these mistakes. We don't have
a time machine, but we can contain this from becoming an ongoing mess
for upstream, and we will do so. It won't be a zoo.

Various proposals have arisen for how to handle quirks in the longer
term, including elaborate frameworks and tables to describe them
generically. I would like to caution against such approaches, especially
in the case that they deviate from practice on x86, or prior to being
standardized fully with other Operating System vendors. I don't expect
there to be too many more than the existing initial set of quirks we
have seen posted. A number of "future" server SoCs have already been
fixed prior to silicon, and new design starts are being warned not to
make this a problem for us to have to clean up later.

So, I would like to suggest that the eventual framework mirror the
existing approach on x86 systems (matching DMI, etc.) and not be made
into some kind of generic, utopia. This is a case where we want there to
be pain involved (and upstream patches required) when people screw up,
so that they have a level of pain in response to ever making this
mistake in the future. If we try to create too grand a generic scheme
and make it too easy to handle this kind of situation beyond the small
number of existing offenders, we undermine efforts to force vendors to
ensure that their IP blocks are compliant going forward.

Side note: if you're a third party IP vendor and we didn't already speak
about this one, drop me a line, and let's collaborate also on your test
frameworks to make sure you're covered as well.

Jon.

--
Computer Architect | Sent from my Fedora powered laptop

Ard Biesheuvel

2016-05-20 07:38:07 UTC

Post by Jon Masters
Hi Tomasz, all,

Post by Tomasz Nowicki
v6 -> v7
- drop quirks handling

Maybe I missed something in the v6 discussion thread; when was it
decided to drop quirk handling?

I had such requests in previous series.

A quick note on quirk handling. This, I believe, applies post-merge of
the base infrastructure, which I realize will likely not have quirks.
We've some "gen1" ARMv8 server platforms where we end up doing quirks
(for things like forcing 32-bit config space accessors and the like) due
to people repurposing existing embedded PCIe IP blocks or using them for
the first time (especially in servers), and those being involved in the
design not necessarily seeing this problem ahead of time, or not
realizing that it would be an issue for servers. In the early days of
ARM server designs 3-4 years ago, many of us had never really played
with ECAM or realized how modern topologies are built.
Anyway. We missed this one in our SBSA requirements. They say (words to
the effect of) "thou shalt do PCIe the way it is done on servers" but
they aren't prescriptive, and they don't tell people how that actually
is in reality. That is being fixed. A lot of things are happening behind
the scenes - especially with third party IP block providers (all of whom
myself and others are speaking with directly about this) - to ensure
that the next wave of designs won't repeat these mistakes. We don't have
a time machine, but we can contain this from becoming an ongoing mess
for upstream, and we will do so. It won't be a zoo.
Various proposals have arisen for how to handle quirks in the longer
term, including elaborate frameworks and tables to describe them
generically. I would like to caution against such approaches, especially
in the case that they deviate from practice on x86, or prior to being
standardized fully with other Operating System vendors. I don't expect
there to be too many more than the existing initial set of quirks we
have seen posted. A number of "future" server SoCs have already been
fixed prior to silicon, and new design starts are being warned not to
make this a problem for us to have to clean up later.
So, I would like to suggest that the eventual framework mirror the
existing approach on x86 systems (matching DMI, etc.) and not be made
into some kind of generic, utopia. This is a case where we want there to
be pain involved (and upstream patches required) when people screw up,
so that they have a level of pain in response to ever making this
mistake in the future. If we try to create too grand a generic scheme
and make it too easy to handle this kind of situation beyond the small
number of existing offenders, we undermine efforts to force vendors to
ensure that their IP blocks are compliant going forward.

I understand that there is a desire from the RedHat side to mimic x86
as closely as possible, but I never saw any technical justification
for that. DMI contains strings that are visible to userland, and you
effectively lock those down to certain values just so that the kernel
can distinguish a broken PCIe root complex from a working one. Linux
on x86 had no choice, since the overwhelming majority of existing
hardware misrepresented itself as generic, and DMI was the only thing
available to actually distinguish these broken implementations from
one another. This does not mean we should allow and/or encourage this
first gen hardware to misrepresent non-compliant hardware as compliant
as well.

Since you are talking to all the people involved, how about you
convince them to put something in the ACPI tables that allows the
kernel to distinguish those non-standard PCIe implementations from
hardware that is really generic? This way, we can sidestep the quirks
debate entirely, since it will simply be a different device as far as
the kernel is concerned. This is no worse than a quirk from a
practical point of view, since an older OS will be equally unable to
run on newer hardware, but it is arguably more true to the standards
compliance you tend to preach about, especially since this small pool
of third party IP could potentially be identified directly rather than
based on some divination of the SoC we may or may not be running on. I
am also convinced that adding support for an additional HID() to the
ACPI ECAM driver with some special config space handling wired in is
an easier sell upstream than making the same ugly mess x86 has had to
make because they did not have any choice to begin with.

If we do need a quirks handling mechanism, I still don't see how the
x86 situation extrapolates to ARM. ACPI offers plenty of ways for a
SoC vendor to identify the make and particular revision, and quirks
could be keyed off of that.

--
Ard.

Jon Masters

2016-05-20 08:01:20 UTC

Hi Ard,

Post by Ard Biesheuvel

Post by Jon Masters
Hi Tomasz, all,

Post by Tomasz Nowicki
v6 -> v7
- drop quirks handling

Maybe I missed something in the v6 discussion thread; when was it
decided to drop quirk handling?

I had such requests in previous series.

A quick note on quirk handling. This, I believe, applies post-merge of
the base infrastructure, which I realize will likely not have quirks.
We've some "gen1" ARMv8 server platforms where we end up doing quirks
(for things like forcing 32-bit config space accessors and the like) due
to people repurposing existing embedded PCIe IP blocks or using them for
the first time (especially in servers), and those being involved in the
design not necessarily seeing this problem ahead of time, or not
realizing that it would be an issue for servers. In the early days of
ARM server designs 3-4 years ago, many of us had never really played
with ECAM or realized how modern topologies are built.
Anyway. We missed this one in our SBSA requirements. They say (words to
the effect of) "thou shalt do PCIe the way it is done on servers" but
they aren't prescriptive, and they don't tell people how that actually
is in reality. That is being fixed. A lot of things are happening behind
the scenes - especially with third party IP block providers (all of whom
myself and others are speaking with directly about this) - to ensure
that the next wave of designs won't repeat these mistakes. We don't have
a time machine, but we can contain this from becoming an ongoing mess
for upstream, and we will do so. It won't be a zoo.
Various proposals have arisen for how to handle quirks in the longer
term, including elaborate frameworks and tables to describe them
generically. I would like to caution against such approaches, especially
in the case that they deviate from practice on x86, or prior to being
standardized fully with other Operating System vendors. I don't expect
there to be too many more than the existing initial set of quirks we
have seen posted. A number of "future" server SoCs have already been
fixed prior to silicon, and new design starts are being warned not to
make this a problem for us to have to clean up later.
So, I would like to suggest that the eventual framework mirror the
existing approach on x86 systems (matching DMI, etc.) and not be made
into some kind of generic, utopia. This is a case where we want there to
be pain involved (and upstream patches required) when people screw up,
so that they have a level of pain in response to ever making this
mistake in the future. If we try to create too grand a generic scheme
and make it too easy to handle this kind of situation beyond the small
number of existing offenders, we undermine efforts to force vendors to
ensure that their IP blocks are compliant going forward.

I understand that there is a desire from the RedHat side to mimic x86
as closely as possible, but I never saw any technical justification
for that.

Understood. My own motivation is always to make the experience as
familiar as possible, both for end users, as well as for ODMs and the
entire ecosystem. There are very many ODMs currently working on v8
server designs and they're already expecting this to be "just like x88".
Intentionally. But as to the specifics of using DMI...

Post by Ard Biesheuvel
DMI contains strings that are visible to userland, and you
effectively lock those down to certain values just so that the kernel
can distinguish a broken PCIe root complex from a working one. Linux
on x86 had no choice, since the overwhelming majority of existing
hardware misrepresented itself as generic, and DMI was the only thing
available to actually distinguish these broken implementations from
one another. This does not mean we should allow and/or encourage this
first gen hardware to misrepresent non-compliant hardware as compliant
as well.

That's a very reasonable argument. I don't disagree that it would be
nice to have nicer ways to distinguish the non-compliant IP than
treating the whole platform with an ASCII matching sledgehammer.

Post by Ard Biesheuvel
Since you are talking to all the people involved, how about you
convince them to put something in the ACPI tables that allows the
kernel to distinguish those non-standard PCIe implementations from
hardware that is really generic?

I'm open to this *BUT* it has to be something that will be adopted
beyond Linux. I have reached out to some non-Linux folks about this. If
there's buy-in, and if there's agreement to go standardize it through
the ASWG, then we should do so. What we should not do is treat ARM as
special in a way that the others aren't involved with. I'll admit DMI
ended up part of the SBBR in part because I wrote that piece in with the
assumption that exactly the same matches as on x86 would happen.

Post by Ard Biesheuvel
This way, we can sidestep the quirks
debate entirely, since it will simply be a different device as far as
the kernel is concerned. This is no worse than a quirk from a
practical point of view, since an older OS will be equally unable to
run on newer hardware, but it is arguably more true to the standards
compliance you tend to preach about, especially since this small pool
of third party IP could potentially be identified directly rather than
based on some divination of the SoC we may or may not be running on. I
am also convinced that adding support for an additional HID() to the
ACPI ECAM driver with some special config space handling wired in is
an easier sell upstream than making the same ugly mess x86 has had to
make because they did not have any choice to begin with.

Again, open to it. I just don't want to do something that's Linux
specific. So it'll take time. It would be awesome if an interim quirk
solution existed that got platforms that are shipping in production
(e.g. HP Moonshot) actually booting upstream kernels this year. We
/really/ want F25 to be able to run on these without needing to carry an
out-of-tree quirk patch or just not support them. That's much worse.

Post by Ard Biesheuvel
If we do need a quirks handling mechanism, I still don't see how the
x86 situation extrapolates to ARM. ACPI offers plenty of ways for a
SoC vendor to identify the make and particular revision, and quirks
could be keyed off of that.

Fair enough. Is there any traction for an interim solution for these
initial platforms do you think? If we wait to add a new table it's
probably going to be the end of the year before we get this done.

Jon.

--
Computer Architect | Sent from my Fedora powered laptop

Ard Biesheuvel

2016-05-20 08:29:19 UTC

Post by Jon Masters
Hi Ard,

Post by Ard Biesheuvel

Post by Jon Masters
Hi Tomasz, all,