[PATCH V1 01/11] x86,

[Tomasz Nowicki]

This patch is the first step for MMCONFIG refactoring process.

Code that uses pci_mmcfg_lock will be moved to common file and become
accessible for all architectures. pci_mmconfig_insert() cannot be moved
so easily since it is mixing generic mmconfig code with x86 specific logic
inside of mutual exclusive block guarded by pci_mmcfg_lock.

To get rid of that constraint, we reorder actions as follow:
1. sanity check for mmconfig region presence, if we already have such
region it doesn't make snese to alloc new mmconfig list entry
2. mmconfig entry allocation, no need to lock
3. insertion to iomem_resource has its own lock, no need to wrap it into mutex
4. insertion to mmconfig list can be done as the final step in separate
function (candidate for further refactoring) and needs another mmconfig
lookup to avoid race condition.

Signed-off-by: Tomasz Nowicki <***@linaro.org>
---
arch/x86/pci/mmconfig-shared.c | 101 +++++++++++++++++++++++------------------
1 file changed, 58 insertions(+), 43 deletions(-)

diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index dd30b7e..c8bb9b0 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -720,6 +720,38 @@ static int __init pci_mmcfg_late_insert_resources(void)
*/
late_initcall(pci_mmcfg_late_insert_resources);

+static int pci_mmconfig_inject(struct pci_mmcfg_region *cfg)
+{
+ struct pci_mmcfg_region *cfg_conflict;
+ int err = 0;
+
+ mutex_lock(&pci_mmcfg_lock);
+ cfg_conflict = pci_mmconfig_lookup(cfg->segment, cfg->start_bus);
+ if (cfg_conflict) {
+ if (cfg_conflict->end_bus < cfg->end_bus)
+ pr_info(FW_INFO "MMCONFIG for "
+ "domain %04x [bus %02x-%02x] "
+ "only partially covers this bridge\n",
+ cfg_conflict->segment, cfg_conflict->start_bus,
+ cfg_conflict->end_bus);
+ err = -EEXIST;
+ goto out;
+ }
+
+ if (pci_mmcfg_arch_map(cfg)) {
+ pr_warn("fail to map MMCONFIG %pR.\n", &cfg->res);
+ err = -ENOMEM;
+ goto out;
+ } else {
+ list_add_sorted(cfg);
+ pr_info("MMCONFIG at %pR (base %#lx)\n",
+ &cfg->res, (unsigned long)cfg->address);
+ }
+out:
+ mutex_unlock(&pci_mmcfg_lock);
+ return err;
+}
+
/* Add MMCFG information for host bridges */
int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
phys_addr_t addr)
@@ -734,63 +766,46 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
if (start > end)
return -EINVAL;

- mutex_lock(&pci_mmcfg_lock);
+ rcu_read_lock();
cfg = pci_mmconfig_lookup(seg, start);
- if (cfg) {
- if (cfg->end_bus < end)
- dev_info(dev, FW_INFO
- "MMCONFIG for "
- "domain %04x [bus %02x-%02x] "
- "only partially covers this bridge\n",
- cfg->segment, cfg->start_bus, cfg->end_bus);
- mutex_unlock(&pci_mmcfg_lock);
+ rcu_read_unlock();
+ if (cfg)
return -EEXIST;
- }

- if (!addr) {
- mutex_unlock(&pci_mmcfg_lock);
+ if (!addr)
return -EINVAL;
- }

- rc = -EBUSY;
cfg = pci_mmconfig_alloc(seg, start, end, addr);
- if (cfg == NULL) {
+ if (!cfg) {
dev_warn(dev, "fail to add MMCONFIG (out of memory)\n");
- rc = -ENOMEM;
- } else if (!pci_mmcfg_check_reserved(dev, cfg, 0)) {
+ return -ENOMEM;
+ }
+
+ rc = -EBUSY;
+ if (!pci_mmcfg_check_reserved(dev, cfg, 0)) {
dev_warn(dev, FW_BUG "MMCONFIG %pR isn't reserved\n",
&cfg->res);
- } else {
- /* Insert resource if it's not in boot stage */
- if (pci_mmcfg_running_state)
- tmp = insert_resource_conflict(&iomem_resource,
- &cfg->res);
-
- if (tmp) {
- dev_warn(dev,
- "MMCONFIG %pR conflicts with "
- "%s %pR\n",
- &cfg->res, tmp->name, tmp);
- } else if (pci_mmcfg_arch_map(cfg)) {
- dev_warn(dev, "fail to map MMCONFIG %pR.\n",
- &cfg->res);
- } else {
- list_add_sorted(cfg);
- dev_info(dev, "MMCONFIG at %pR (base %#lx)\n",
- &cfg->res, (unsigned long)addr);
- cfg = NULL;
- rc = 0;
- }
+ goto error;
}

- if (cfg) {
- if (cfg->res.parent)
- release_resource(&cfg->res);
- kfree(cfg);
+ /* Insert resource if it's not in boot stage */
+ if (pci_mmcfg_running_state)
+ tmp = insert_resource_conflict(&iomem_resource, &cfg->res);
+
+ if (tmp) {
+ dev_warn(dev, "MMCONFIG %pR conflicts with %s %pR\n",
+ &cfg->res, tmp->name, tmp);
+ goto error;
}

- mutex_unlock(&pci_mmcfg_lock);
+ rc = pci_mmconfig_inject(cfg);
+ if (!rc)
+ return 0;

+error:
+ if (cfg->res.parent)
+ release_resource(&cfg->res);
+ kfree(cfg);
return rc;
}

[Tomasz Nowicki]

mmconfig_64.c version is going to be default implementation for low-level
operation on mmconfig regions. However, now it initializes raw_pci_ext_ops pointer which is specific for
x86 only. Moreover, mmconfig_32.c is doing the same thing at the same time.
So lets move it to mmconfig_shared.c so it becomes common for both and
mmconfig_64.c turns out to be purely arch agnostic.

Signed-off-by: Tomasz Nowicki <***@semihalf.com>
---
arch/x86/include/asm/pci_x86.h | 5 +++++
arch/x86/pci/mmconfig-shared.c | 10 ++++++++--
arch/x86/pci/mmconfig_32.c | 10 ++--------
arch/x86/pci/mmconfig_64.c | 11 ++---------
4 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h
index 64cb514..039f69e 100644
--- a/arch/x86/include/asm/pci_x86.h
+++ b/arch/x86/include/asm/pci_x86.h
@@ -129,6 +129,11 @@ extern void pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg);
extern int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
phys_addr_t addr);

+int pci_mmcfg_read(unsigned int seg, unsigned int bus, unsigned int devfn,
+ int reg, int len, u32 *value);
+int pci_mmcfg_write(unsigned int seg, unsigned int bus, unsigned int devfn,
+ int reg, int len, u32 value);
+
/*
* AMD Fam10h CPUs are buggy, and cannot access MMIO config space
* on their northbrige except through the * %eax register. As such, you MUST
diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index ce2c2e4..980f304 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -29,6 +29,11 @@
static bool pci_mmcfg_running_state;
static bool pci_mmcfg_arch_init_failed;

+const struct pci_raw_ops pci_mmcfg = {
+ .read = pci_mmcfg_read,
+ .write = pci_mmcfg_write,
+};
+
static const char *__init pci_mmcfg_e7520(void)
{
u32 win;
@@ -512,9 +517,10 @@ static void __init __pci_mmcfg_init(int early)
}
}

- if (pci_mmcfg_arch_init())
+ if (pci_mmcfg_arch_init()) {
+ raw_pci_ext_ops = &pci_mmcfg;
pci_probe = (pci_probe & ~PCI_PROBE_MASK) | PCI_PROBE_MMCONF;
- else {
+ } else {
free_all_mmcfg();
pci_mmcfg_arch_init_failed = true;
}
diff --git a/arch/x86/pci/mmconfig_32.c b/arch/x86/pci/mmconfig_32.c
index 246f135..2ded56f 100644
--- a/arch/x86/pci/mmconfig_32.c
+++ b/arch/x86/pci/mmconfig_32.c
@@ -50,7 +50,7 @@ static void pci_exp_set_dev_base(unsigned int base, int bus, int devfn)
}
}

-static int pci_mmcfg_read(unsigned int seg, unsigned int bus,
+int pci_mmcfg_read(unsigned int seg, unsigned int bus,
unsigned int devfn, int reg, int len, u32 *value)
{
unsigned long flags;
@@ -89,7 +89,7 @@ err: *value = -1;
return 0;
}

-static int pci_mmcfg_write(unsigned int seg, unsigned int bus,
+int pci_mmcfg_write(unsigned int seg, unsigned int bus,
unsigned int devfn, int reg, int len, u32 value)
{
unsigned long flags;
@@ -126,15 +126,9 @@ static int pci_mmcfg_write(unsigned int seg, unsigned int bus,
return 0;
}

-const struct pci_raw_ops pci_mmcfg = {
- .read = pci_mmcfg_read,
- .write = pci_mmcfg_write,
-};
-
int __init pci_mmcfg_arch_init(void)
{
printk(KERN_INFO "PCI: Using MMCONFIG for extended config space\n");
- raw_pci_ext_ops = &pci_mmcfg;
return 1;
}

diff --git a/arch/x86/pci/mmconfig_64.c b/arch/x86/pci/mmconfig_64.c
index b14fcd3..d0c48eb 100644
--- a/arch/x86/pci/mmconfig_64.c
+++ b/arch/x86/pci/mmconfig_64.c
@@ -25,7 +25,7 @@ static char __iomem *pci_dev_base(unsigned int seg, unsigned int bus, unsigned i
return NULL;
}

-static int pci_mmcfg_read(unsigned int seg, unsigned int bus,
+int pci_mmcfg_read(unsigned int seg, unsigned int bus,
unsigned int devfn, int reg, int len, u32 *value)
{
char __iomem *addr;
@@ -59,7 +59,7 @@ err: *value = -1;
return 0;
}

-static int pci_mmcfg_write(unsigned int seg, unsigned int bus,
+int pci_mmcfg_write(unsigned int seg, unsigned int bus,
unsigned int devfn, int reg, int len, u32 value)
{
char __iomem *addr;
@@ -91,11 +91,6 @@ static int pci_mmcfg_write(unsigned int seg, unsigned int bus,
return 0;
}

-const struct pci_raw_ops pci_mmcfg = {
- .read = pci_mmcfg_read,
- .write = pci_mmcfg_write,
-};
-
static void __iomem *mcfg_ioremap(struct pci_mmcfg_region *cfg)
{
void __iomem *addr;
@@ -121,8 +116,6 @@ int __init pci_mmcfg_arch_init(void)
return 0;
}

- raw_pci_ext_ops = &pci_mmcfg;
-
return 1;
}

[Tomasz Nowicki]

Lets keep RAW ACPI PCI config space accessors empty by default,
since we are note sure if they are necessary accross all archs.
Once we sort this out, we can provide generic version or let
architectures to overwrite, like now x86.

Signed-off-by: Tomasz Nowicki <***@semihalf.com>
---
drivers/acpi/mcfg.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)

diff --git a/drivers/acpi/mcfg.c b/drivers/acpi/mcfg.c
index 745b83e..3e1e7be 100644
--- a/drivers/acpi/mcfg.c
+++ b/drivers/acpi/mcfg.c
@@ -9,9 +9,30 @@

#include <linux/acpi.h>
#include <linux/ecam.h>
+#include <linux/pci.h>

#define PREFIX "MCFG: "

+/*
+ * raw_pci_read/write - raw ACPI PCI config space accessors.
+ *
+ * By defauly (__weak) these accessors are empty and should be overwritten
+ * by architectures which support operations on ACPI PCI_Config regions,
+ * see osl.c file.
+ */
+
+int __weak raw_pci_read(unsigned int domain, unsigned int bus,
+ unsigned int devfn, int reg, int len, u32 *val)
+{
+ return PCIBIOS_DEVICE_NOT_FOUND;
+}
+
+int __weak raw_pci_write(unsigned int domain, unsigned int bus,
+ unsigned int devfn, int reg, int len, u32 val)
+{
+ return PCIBIOS_DEVICE_NOT_FOUND;
+}
+
int __init acpi_parse_mcfg(struct acpi_table_header *header)
{
struct acpi_table_mcfg *mcfg;

[Tomasz Nowicki]

This post might be inappropriate. Click to display it.

[Tomasz Nowicki]

Now that we have hot_added flag we can get rid of arch specific mcfg_added.

Signed-off-by: Tomasz Nowicki <***@semihalf.com>
---
arch/x86/pci/acpi.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index 28c9cd6..b4ef761 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -13,7 +13,6 @@ struct pci_root_info {
struct acpi_pci_root_info common;
struct pci_sysdata sd;
#ifdef CONFIG_PCI_MMCONFIG
- bool mcfg_added;
u8 start_bus;
u8 end_bus;
#endif
@@ -188,7 +187,6 @@ static int setup_mcfg_map(struct acpi_pci_root_info *ci)
info = container_of(ci, struct pci_root_info, common);
info->start_bus = (u8)root->secondary.start;
info->end_bus = (u8)root->secondary.end;
- info->mcfg_added = false;
seg = info->sd.domain;

/* return success if MMCFG is not in use */
@@ -204,7 +202,6 @@ static int setup_mcfg_map(struct acpi_pci_root_info *ci)
/* enable MMCFG if it hasn't been enabled yet */
if (raw_pci_ext_ops == NULL)
raw_pci_ext_ops = &pci_mmcfg;
- info->mcfg_added = true;
} else if (result != -EEXIST)
return check_segment(seg, dev,
"fail to add MMCONFIG information,");
@@ -214,14 +211,17 @@ static int setup_mcfg_map(struct acpi_pci_root_info *ci)

static void teardown_mcfg_map(struct acpi_pci_root_info *ci)
{
+ struct pci_mmcfg_region *cfg;
struct pci_root_info *info;

info = container_of(ci, struct pci_root_info, common);
- if (info->mcfg_added) {
- pci_mmconfig_delete(info->sd.domain,
- info->start_bus, info->end_bus);
- info->mcfg_added = false;
- }
+ cfg = pci_mmconfig_lookup(info->sd.domain, info->start_bus);
+ if (!cfg)
+ return;
+
+ if (cfg->hot_added)
+ pci_mmconfig_delete(info->sd.domain, info->start_bus,
+ info->end_bus);
}
#else
static int setup_mcfg_map(struct acpi_pci_root_info *ci)

[Tomasz Nowicki]

There are two ways we can get ECAM (aka MCFG) regions using ACPI,
from MCFG static table and from _CBA method. We cannot remove static
regions, however regions coming from _CBA should be removed while removing
bridge device.

In the light of above we need flag to mark hot added ECAM entries
so that user should use pci_mmconfig_inject while adding regions from
_CBA method.

Signed-off-by: Tomasz Nowicki <***@semihalf.com>
---
drivers/pci/ecam.c | 2 ++
include/linux/ecam.h | 1 +
2 files changed, 3 insertions(+)

diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
index 8a5eef7..da35b4c 100644
--- a/drivers/pci/ecam.c
+++ b/drivers/pci/ecam.c
@@ -131,6 +131,7 @@ struct pci_mmcfg_region *pci_mmconfig_alloc(int segment, int start,
new->segment = segment;
new->start_bus = start;
new->end_bus = end;
+ new->hot_added = false;

res = &new->res;
res->start = addr + PCI_MMCFG_BUS_OFFSET(start);
@@ -221,6 +222,7 @@ int pci_mmconfig_inject(struct pci_mmcfg_region *cfg)
err = -ENOMEM;
goto out;
} else {
+ cfg->hot_added = true;
list_add_sorted(cfg);
pr_info("MMCONFIG at %pR (base %#lx)\n",
&cfg->res, (unsigned long)cfg->address);
diff --git a/include/linux/ecam.h b/include/linux/ecam.h
index 813acd1..e0f322e 100644
--- a/include/linux/ecam.h
+++ b/include/linux/ecam.h
@@ -17,6 +17,7 @@ struct pci_mmcfg_region {
u8 start_bus;
u8 end_bus;
char name[PCI_MMCFG_RESOURCE_NAME_LEN];
+ bool hot_added;
};

struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus);

[Tomasz Nowicki]

Architectures which support PCI_DOMAINS_GENERIC (like ARM64)
cannot call pci_bus_assign_domain_nr along ACPI PCI host bridge
initialization since this function needs valid parent device reference
to be able to retrieve domain number (aka segment).

We can omit that blocker and pass down host bridge device via
pci_create_root_bus parameter and then be able to evaluate _SEG method
being in pci_bus_assign_domain_nr.

Note that _SEG method is optional, therefore _SEG absence means
that all PCI buses belong to domain 0.

Signed-off-by: Tomasz Nowicki <***@semihalf.com>
---
drivers/acpi/pci_root.c | 2 +-
drivers/pci/pci.c | 32 +++++++++++++++++++++++++++-----
2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 850d7bf..e682dc6 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -839,7 +839,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,

pci_acpi_root_add_resources(info);
pci_add_resource(&info->resources, &root->secondary);
- bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
+ bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops,
sysdata, &info->resources);
if (!bus)
goto out_release_info;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6a9a111..17d1857 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/acpi.h>
#include <asm-generic/pci-bridge.h>
#include <asm/setup.h>
#include "pci.h"
@@ -4501,7 +4502,7 @@ int pci_get_new_domain_nr(void)
void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
{
static int use_dt_domains = -1;
- int domain = of_get_pci_domain_nr(parent->of_node);
+ int domain;

/*
* Check DT domain and use_dt_domains values.
@@ -4523,14 +4524,35 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
* API and update the use_dt_domains value to keep track of method we
* are using to assign domain numbers (use_dt_domains = 0).
*
+ * IF ACPI, we expect non-DT method (use_dt_domains == -1)
+ * and call _SEG method for corresponding host bridge device.
+ * If _SEG method does not exist, following ACPI spec (6.5.6)
+ * all PCI buses belong to domain 0.
+ *
* All other combinations imply we have a platform that is trying
- * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
- * which is a recipe for domain mishandling and it is prevented by
- * invalidating the domain value (domain = -1) and printing a
- * corresponding error.
+ * to mix domain numbers obtained from DT, ACPI and
+ * pci_get_new_domain_nr(), which is a recipe for domain mishandling and
+ * it is prevented by invalidating the domain value (domain = -1) and
+ * printing a corresponding error.
*/
+
+ domain = of_get_pci_domain_nr(parent->of_node);
if (domain >= 0 && use_dt_domains) {
use_dt_domains = 1;
+#ifdef CONFIG_ACPI
+ } else if (!acpi_disabled && use_dt_domains == -1) {
+ struct acpi_device *acpi_dev = to_acpi_device(parent);
+ unsigned long long segment = 0;
+ acpi_status status;
+
+ status = acpi_evaluate_integer(acpi_dev->handle,
+ METHOD_NAME__SEG, NULL,
+ &segment);
+ if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
+ dev_err(&acpi_dev->dev, "can't evaluate _SEG\n");
+
+ domain = segment;
+#endif
} else if (domain < 0 && use_dt_domains != 1) {
use_dt_domains = 0;
domain = pci_get_new_domain_nr();

[L***@arm.com]

Post by Tomasz Nowicki
Architectures which support PCI_DOMAINS_GENERIC (like ARM64)
cannot call pci_bus_assign_domain_nr along ACPI PCI host bridge
initialization since this function needs valid parent device reference
to be able to retrieve domain number (aka segment).
We can omit that blocker and pass down host bridge device via
pci_create_root_bus parameter and then be able to evaluate _SEG method
being in pci_bus_assign_domain_nr.
Note that _SEG method is optional, therefore _SEG absence means
that all PCI buses belong to domain 0.
---
drivers/acpi/pci_root.c | 2 +-
drivers/pci/pci.c | 32 +++++++++++++++++++++++++++-----
2 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 850d7bf..e682dc6 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -839,7 +839,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
pci_acpi_root_add_resources(info);
pci_add_resource(&info->resources, &root->secondary);
- bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
+ bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops,
sysdata, &info->resources);

Not sure this change should be in this patch, I don't see the relation.

To put it differently: I think the patch should introduce the retrieval of the
domain number from _SEG method and leave the passing of a valid host bridge device
to a more appropriate patch.

Post by Tomasz Nowicki
if (!bus)
goto out_release_info;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6a9a111..17d1857 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/acpi.h>
#include <asm-generic/pci-bridge.h>
#include <asm/setup.h>
#include "pci.h"
@@ -4501,7 +4502,7 @@ int pci_get_new_domain_nr(void)
void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
{
static int use_dt_domains = -1;
- int domain = of_get_pci_domain_nr(parent->of_node);
+ int domain;
/*
* Check DT domain and use_dt_domains values.
@@ -4523,14 +4524,35 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
* API and update the use_dt_domains value to keep track of method we
* are using to assign domain numbers (use_dt_domains = 0).
*
+ * IF ACPI, we expect non-DT method (use_dt_domains == -1)
+ * and call _SEG method for corresponding host bridge device.
+ * If _SEG method does not exist, following ACPI spec (6.5.6)
+ * all PCI buses belong to domain 0.
+ *
* All other combinations imply we have a platform that is trying
- * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
- * which is a recipe for domain mishandling and it is prevented by
- * invalidating the domain value (domain = -1) and printing a
- * corresponding error.
+ * to mix domain numbers obtained from DT, ACPI and
+ * pci_get_new_domain_nr(), which is a recipe for domain mishandling and
+ * it is prevented by invalidating the domain value (domain = -1) and
+ * printing a corresponding error.
*/
+
+ domain = of_get_pci_domain_nr(parent->of_node);

Not sure what you've got here by splitting the original line into two other than an increase
in the change count.

Otherwise, it looks sensible.

Post by Tomasz Nowicki
if (domain >= 0 && use_dt_domains) {
use_dt_domains = 1;
+#ifdef CONFIG_ACPI
+ } else if (!acpi_disabled && use_dt_domains == -1) {
+ struct acpi_device *acpi_dev = to_acpi_device(parent);
+ unsigned long long segment = 0;
+ acpi_status status;
+
+ status = acpi_evaluate_integer(acpi_dev->handle,
+ METHOD_NAME__SEG, NULL,
+ &segment);
+ if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
+ dev_err(&acpi_dev->dev, "can't evaluate _SEG\n");
+
+ domain = segment;
+#endif
} else if (domain < 0 && use_dt_domains != 1) {
use_dt_domains = 0;
domain = pci_get_new_domain_nr();
--
1.9.1

[Tomasz Nowicki]

Hi Liviu,

Post by L***@arm.com

Post by Tomasz Nowicki
Architectures which support PCI_DOMAINS_GENERIC (like ARM64)
cannot call pci_bus_assign_domain_nr along ACPI PCI host bridge
initialization since this function needs valid parent device reference
to be able to retrieve domain number (aka segment).
We can omit that blocker and pass down host bridge device via
pci_create_root_bus parameter and then be able to evaluate _SEG method
being in pci_bus_assign_domain_nr.
Note that _SEG method is optional, therefore _SEG absence means
that all PCI buses belong to domain 0.
---
drivers/acpi/pci_root.c | 2 +-
drivers/pci/pci.c | 32 +++++++++++++++++++++++++++-----
2 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 850d7bf..e682dc6 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -839,7 +839,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
pci_acpi_root_add_resources(info);
pci_add_resource(&info->resources, &root->secondary);
- bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
+ bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops,
sysdata, &info->resources);

Not sure this change should be in this patch, I don't see the relation.
To put it differently: I think the patch should introduce the retrieval of the
domain number from _SEG method and leave the passing of a valid host bridge device
to a more appropriate patch.

I wanted to highlight that ACPI kernel using PCI_DOMAINS_GENERIC needs
to have both in place:
1. Obtaining domain from _SEG method
2. And host bridge device reference for which we can call _SEG.
But you are right, it will be more clear if I split up patch and
describe it in separate changelog.

Post by L***@arm.com

Post by Tomasz Nowicki
if (!bus)
goto out_release_info;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6a9a111..17d1857 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/acpi.h>
#include <asm-generic/pci-bridge.h>
#include <asm/setup.h>
#include "pci.h"
@@ -4501,7 +4502,7 @@ int pci_get_new_domain_nr(void)
void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
{
static int use_dt_domains = -1;
- int domain = of_get_pci_domain_nr(parent->of_node);
+ int domain;
/*
* Check DT domain and use_dt_domains values.
@@ -4523,14 +4524,35 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
* API and update the use_dt_domains value to keep track of method we
* are using to assign domain numbers (use_dt_domains = 0).
*
+ * IF ACPI, we expect non-DT method (use_dt_domains == -1)
+ * and call _SEG method for corresponding host bridge device.
+ * If _SEG method does not exist, following ACPI spec (6.5.6)
+ * all PCI buses belong to domain 0.
+ *
* All other combinations imply we have a platform that is trying
- * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
- * which is a recipe for domain mishandling and it is prevented by
- * invalidating the domain value (domain = -1) and printing a
- * corresponding error.
+ * to mix domain numbers obtained from DT, ACPI and
+ * pci_get_new_domain_nr(), which is a recipe for domain mishandling and
+ * it is prevented by invalidating the domain value (domain = -1) and
+ * printing a corresponding error.
*/
+
+ domain = of_get_pci_domain_nr(parent->of_node);

Not sure what you've got here by splitting the original line into two other than an increase
in the change count.

Yes, it does not make sense to split the original line. I will fix that.

Post by L***@arm.com
Otherwise, it looks sensible.

Thanks Liviu!

Regards,
Tomasz

[Lorenzo Pieralisi]

Post by Tomasz Nowicki
Architectures which support PCI_DOMAINS_GENERIC (like ARM64)
cannot call pci_bus_assign_domain_nr along ACPI PCI host bridge
initialization since this function needs valid parent device reference
to be able to retrieve domain number (aka segment).
We can omit that blocker and pass down host bridge device via
pci_create_root_bus parameter and then be able to evaluate _SEG method
being in pci_bus_assign_domain_nr.
Note that _SEG method is optional, therefore _SEG absence means
that all PCI buses belong to domain 0.
---
drivers/acpi/pci_root.c | 2 +-
drivers/pci/pci.c | 32 +++++++++++++++++++++++++++-----
2 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 850d7bf..e682dc6 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -839,7 +839,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
pci_acpi_root_add_resources(info);
pci_add_resource(&info->resources, &root->secondary);
- bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
+ bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops,
sysdata, &info->resources);

If I read x86 code correctly, they rely on the first argument to be
NULL, I think you would break x86 by doing that, see:

arch/x86/pci/acpi.c (pcibios_root_bridge_prepare())

By the way, can't we move the code setting up the ACPI_COMPANION
to core PCI code and stop relying on sysdata for that ?

Thanks,
Lorenzo

Post by Tomasz Nowicki
if (!bus)
goto out_release_info;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6a9a111..17d1857 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/acpi.h>
#include <asm-generic/pci-bridge.h>
#include <asm/setup.h>
#include "pci.h"
@@ -4501,7 +4502,7 @@ int pci_get_new_domain_nr(void)
void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
{
static int use_dt_domains = -1;
- int domain = of_get_pci_domain_nr(parent->of_node);
+ int domain;
/*
* Check DT domain and use_dt_domains values.
@@ -4523,14 +4524,35 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
* API and update the use_dt_domains value to keep track of method we
* are using to assign domain numbers (use_dt_domains = 0).
*
+ * IF ACPI, we expect non-DT method (use_dt_domains == -1)
+ * and call _SEG method for corresponding host bridge device.
+ * If _SEG method does not exist, following ACPI spec (6.5.6)
+ * all PCI buses belong to domain 0.
+ *
* All other combinations imply we have a platform that is trying
- * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
- * which is a recipe for domain mishandling and it is prevented by
- * invalidating the domain value (domain = -1) and printing a
- * corresponding error.
+ * to mix domain numbers obtained from DT, ACPI and
+ * pci_get_new_domain_nr(), which is a recipe for domain mishandling and
+ * it is prevented by invalidating the domain value (domain = -1) and
+ * printing a corresponding error.
*/
+
+ domain = of_get_pci_domain_nr(parent->of_node);
if (domain >= 0 && use_dt_domains) {
use_dt_domains = 1;
+#ifdef CONFIG_ACPI
+ } else if (!acpi_disabled && use_dt_domains == -1) {
+ struct acpi_device *acpi_dev = to_acpi_device(parent);
+ unsigned long long segment = 0;
+ acpi_status status;
+
+ status = acpi_evaluate_integer(acpi_dev->handle,
+ METHOD_NAME__SEG, NULL,
+ &segment);
+ if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
+ dev_err(&acpi_dev->dev, "can't evaluate _SEG\n");
+
+ domain = segment;
+#endif
} else if (domain < 0 && use_dt_domains != 1) {
use_dt_domains = 0;
domain = pci_get_new_domain_nr();
--
1.9.1

[Tomasz Nowicki]

Post by Lorenzo Pieralisi

Post by Tomasz Nowicki
Architectures which support PCI_DOMAINS_GENERIC (like ARM64)
cannot call pci_bus_assign_domain_nr along ACPI PCI host bridge
initialization since this function needs valid parent device reference
to be able to retrieve domain number (aka segment).
We can omit that blocker and pass down host bridge device via
pci_create_root_bus parameter and then be able to evaluate _SEG method
being in pci_bus_assign_domain_nr.
Note that _SEG method is optional, therefore _SEG absence means
that all PCI buses belong to domain 0.
---
drivers/acpi/pci_root.c | 2 +-
drivers/pci/pci.c | 32 +++++++++++++++++++++++++++-----
2 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 850d7bf..e682dc6 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -839,7 +839,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
pci_acpi_root_add_resources(info);
pci_add_resource(&info->resources, &root->secondary);
- bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
+ bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops,
sysdata, &info->resources);

If I read x86 code correctly, they rely on the first argument to be
arch/x86/pci/acpi.c (pcibios_root_bridge_prepare())
By the way, can't we move the code setting up the ACPI_COMPANION
to core PCI code and stop relying on sysdata for that ?

Yes, that will break x86&ia64 but as you said it may be overcome with
consolidation of ACPI_COMPANION into the core code.

Tomasz

[Tomasz Nowicki]

First function acpi_mcfg_check_entry() does not apply any quirks by default.

Last two functions are required by ACPI subsystem to make PCI config
space accessible. Generic code assume to do nothing for early init call but
late init call does as follow:
- parse MCFG table and add regions to ECAM resource list
- map regions
- add regions to iomem_resource

Signed-off-by: Tomasz Nowicki <***@semihalf.com>
---
drivers/acpi/mcfg.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)

diff --git a/drivers/acpi/mcfg.c b/drivers/acpi/mcfg.c
index 5ecef20..fad9917 100644
--- a/drivers/acpi/mcfg.c
+++ b/drivers/acpi/mcfg.c
@@ -57,3 +57,29 @@ int __init acpi_parse_mcfg(struct acpi_table_header *header)

return 0;
}
+
+int __init __weak acpi_mcfg_check_entry(struct acpi_table_mcfg *mcfg,
+ struct acpi_mcfg_allocation *cfg)
+{
+ return 0;
+}
+
+void __init __weak pci_mmcfg_early_init(void)
+{
+
+}
+
+void __init __weak pci_mmcfg_late_init(void)
+{
+ struct pci_mmcfg_region *cfg;
+
+ acpi_table_parse(ACPI_SIG_MCFG, acpi_parse_mcfg);
+
+ if (list_empty(&pci_mmcfg_list))
+ return;
+ if (!pci_mmcfg_arch_init())
+ free_all_mmcfg();
+
+ list_for_each_entry(cfg, &pci_mmcfg_list, list)
+ insert_resource(&iomem_resource, &cfg->res);
+}

[Tomasz Nowicki]

This post might be inappropriate. Click to display it.

[Tomasz Nowicki]

From: Hanjun Guo <***@linaro.org>

In drivers/xen/pci.c, there are arch x86 dependent codes when
CONFIG_PCI_MMCONFIG is enabled, since CONFIG_PCI_MMCONFIG
depends on ACPI, so this will prevent XEN PCI running on other
architectures using ACPI with PCI_MMCONFIG enabled (such as ARM64).

Fortunatly, it can be sloved in a simple way. In drivers/xen/pci.c,
the only x86 dependent code is if ((pci_probe & PCI_PROBE_MMCONF) == 0),
and it's defined in asm/pci_x86.h, the code means that
if the PCI resource is not probed in PCI_PROBE_MMCONF way, just
ingnore the xen mcfg init. Actually this is duplicate, because
if PCI resource is not probed in PCI_PROBE_MMCONF way, the
pci_mmconfig_list will be empty, and the if (list_empty())
after it will do the same job.

So just remove the arch related code and the head file, this
will be no functional change for x86, and also makes xen/pci.c
usable for other architectures.

Signed-off-by: Hanjun Guo <***@linaro.org>
CC: Konrad Rzeszutek Wilk <***@oracle.com>
CC: Boris Ostrovsky <***@oracle.com>
---
drivers/xen/pci.c | 6 ------
1 file changed, 6 deletions(-)

diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
index 6785ebb..9a8dbe3 100644
--- a/drivers/xen/pci.c
+++ b/drivers/xen/pci.c
@@ -28,9 +28,6 @@
#include <asm/xen/hypervisor.h>
#include <asm/xen/hypercall.h>
#include "../pci/pci.h"
-#ifdef CONFIG_PCI_MMCONFIG
-#include <asm/pci_x86.h>
-#endif

static bool __read_mostly pci_seg_supported = true;

@@ -222,9 +219,6 @@ static int __init xen_mcfg_late(void)
if (!xen_initial_domain())
return 0;

- if ((pci_probe & PCI_PROBE_MMCONF) == 0)
- return 0;
-
if (list_empty(&pci_mmcfg_list))
return 0;

[Tomasz Nowicki]

+ Stefano

Post by Tomasz Nowicki
In drivers/xen/pci.c, there are arch x86 dependent codes when
CONFIG_PCI_MMCONFIG is enabled, since CONFIG_PCI_MMCONFIG
depends on ACPI, so this will prevent XEN PCI running on other
architectures using ACPI with PCI_MMCONFIG enabled (such as ARM64).
Fortunatly, it can be sloved in a simple way. In drivers/xen/pci.c,
the only x86 dependent code is if ((pci_probe & PCI_PROBE_MMCONF) == 0),
and it's defined in asm/pci_x86.h, the code means that
if the PCI resource is not probed in PCI_PROBE_MMCONF way, just
ingnore the xen mcfg init. Actually this is duplicate, because
if PCI resource is not probed in PCI_PROBE_MMCONF way, the
pci_mmconfig_list will be empty, and the if (list_empty())
after it will do the same job.
So just remove the arch related code and the head file, this
will be no functional change for x86, and also makes xen/pci.c
usable for other architectures.
---
drivers/xen/pci.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
index 6785ebb..9a8dbe3 100644
--- a/drivers/xen/pci.c
+++ b/drivers/xen/pci.c
@@ -28,9 +28,6 @@
#include <asm/xen/hypervisor.h>
#include <asm/xen/hypercall.h>
#include "../pci/pci.h"
-#ifdef CONFIG_PCI_MMCONFIG
-#include <asm/pci_x86.h>
-#endif
static bool __read_mostly pci_seg_supported = true;
@@ -222,9 +219,6 @@ static int __init xen_mcfg_late(void)
if (!xen_initial_domain())
return 0;
- if ((pci_probe & PCI_PROBE_MMCONF) == 0)
- return 0;
-
if (list_empty(&pci_mmcfg_list))
return 0;

[Boris Ostrovsky]

Post by Tomasz Nowicki
+ Stefano

Post by Tomasz Nowicki
In drivers/xen/pci.c, there are arch x86 dependent codes when
CONFIG_PCI_MMCONFIG is enabled, since CONFIG_PCI_MMCONFIG
depends on ACPI, so this will prevent XEN PCI running on other
architectures using ACPI with PCI_MMCONFIG enabled (such as ARM64).
Fortunatly, it can be sloved in a simple way. In drivers/xen/pci.c,
the only x86 dependent code is if ((pci_probe & PCI_PROBE_MMCONF) == 0),
and it's defined in asm/pci_x86.h, the code means that
if the PCI resource is not probed in PCI_PROBE_MMCONF way, just
ingnore the xen mcfg init. Actually this is duplicate, because
if PCI resource is not probed in PCI_PROBE_MMCONF way, the
pci_mmconfig_list will be empty, and the if (list_empty())
after it will do the same job.
So just remove the arch related code and the head file, this
will be no functional change for x86, and also makes xen/pci.c
usable for other architectures.
---
drivers/xen/pci.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
index 6785ebb..9a8dbe3 100644
--- a/drivers/xen/pci.c
+++ b/drivers/xen/pci.c
@@ -28,9 +28,6 @@
#include <asm/xen/hypervisor.h>
#include <asm/xen/hypercall.h>
#include "../pci/pci.h"
-#ifdef CONFIG_PCI_MMCONFIG
-#include <asm/pci_x86.h>
-#endif

Assuming this still compiles on x86 now that this include file is removed

Post by Tomasz Nowicki

Post by Tomasz Nowicki
static bool __read_mostly pci_seg_supported = true;
@@ -222,9 +219,6 @@ static int __init xen_mcfg_late(void)
if (!xen_initial_domain())
return 0;
- if ((pci_probe & PCI_PROBE_MMCONF) == 0)
- return 0;
-
if (list_empty(&pci_mmcfg_list))
return 0;

[Stefano Stabellini]

Post by Boris Ostrovsky

Post by Tomasz Nowicki
+ Stefano

Post by Tomasz Nowicki
In drivers/xen/pci.c, there are arch x86 dependent codes when
CONFIG_PCI_MMCONFIG is enabled, since CONFIG_PCI_MMCONFIG
depends on ACPI, so this will prevent XEN PCI running on other
architectures using ACPI with PCI_MMCONFIG enabled (such as ARM64).
Fortunatly, it can be sloved in a simple way. In drivers/xen/pci.c,
the only x86 dependent code is if ((pci_probe & PCI_PROBE_MMCONF) == 0),
and it's defined in asm/pci_x86.h, the code means that
if the PCI resource is not probed in PCI_PROBE_MMCONF way, just
ingnore the xen mcfg init. Actually this is duplicate, because
if PCI resource is not probed in PCI_PROBE_MMCONF way, the
pci_mmconfig_list will be empty, and the if (list_empty())
after it will do the same job.
So just remove the arch related code and the head file, this
will be no functional change for x86, and also makes xen/pci.c
usable for other architectures.
---
drivers/xen/pci.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
index 6785ebb..9a8dbe3 100644
--- a/drivers/xen/pci.c
+++ b/drivers/xen/pci.c
@@ -28,9 +28,6 @@
#include <asm/xen/hypervisor.h>
#include <asm/xen/hypercall.h>
#include "../pci/pci.h"
-#ifdef CONFIG_PCI_MMCONFIG
-#include <asm/pci_x86.h>
-#endif

Assuming this still compiles on x86 now that this include file is removed

I think it does not:

drivers/xen/pci.c: In function ‘xen_mcfg_late’:
drivers/xen/pci.c:221:18: error: ‘pci_mmcfg_list’ undeclared (first use in this function)
drivers/xen/pci.c:221:18: note: each undeclared identifier is reported only once for each f

Post by Boris Ostrovsky

Post by Tomasz Nowicki

Post by Tomasz Nowicki
static bool __read_mostly pci_seg_supported = true;
@@ -222,9 +219,6 @@ static int __init xen_mcfg_late(void)
if (!xen_initial_domain())
return 0;
- if ((pci_probe & PCI_PROBE_MMCONF) == 0)
- return 0;
-
if (list_empty(&pci_mmcfg_list))
return 0;

[Tomasz Nowicki]

Post by Boris Ostrovsky

Post by Tomasz Nowicki
+ Stefano

Post by Tomasz Nowicki
In drivers/xen/pci.c, there are arch x86 dependent codes when
CONFIG_PCI_MMCONFIG is enabled, since CONFIG_PCI_MMCONFIG
depends on ACPI, so this will prevent XEN PCI running on other
architectures using ACPI with PCI_MMCONFIG enabled (such as ARM64).
Fortunatly, it can be sloved in a simple way. In drivers/xen/pci.c,
the only x86 dependent code is if ((pci_probe & PCI_PROBE_MMCONF) == 0),
and it's defined in asm/pci_x86.h, the code means that
if the PCI resource is not probed in PCI_PROBE_MMCONF way, just
ingnore the xen mcfg init. Actually this is duplicate, because
if PCI resource is not probed in PCI_PROBE_MMCONF way, the
pci_mmconfig_list will be empty, and the if (list_empty())
after it will do the same job.
So just remove the arch related code and the head file, this
will be no functional change for x86, and also makes xen/pci.c
usable for other architectures.
---
drivers/xen/pci.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
index 6785ebb..9a8dbe3 100644
--- a/drivers/xen/pci.c
+++ b/drivers/xen/pci.c
@@ -28,9 +28,6 @@
#include <asm/xen/hypervisor.h>
#include <asm/xen/hypercall.h>
#include "../pci/pci.h"
-#ifdef CONFIG_PCI_MMCONFIG
-#include <asm/pci_x86.h>
-#endif

Assuming this still compiles on x86 now that this include file is removed

drivers/xen/pci.c:221:18: error: ‘pci_mmcfg_list’ undeclared (first use in this function)
drivers/xen/pci.c:221:18: note: each undeclared identifier is reported only once for each f

Right, we need:
+#include <linux/ecam.h>

Will fix this in next version, thanks!

Tomasz

Post by Boris Ostrovsky

Post by Tomasz Nowicki

Post by Tomasz Nowicki
static bool __read_mostly pci_seg_supported = true;
@@ -222,9 +219,6 @@ static int __init xen_mcfg_late(void)
if (!xen_initial_domain())
return 0;
- if ((pci_probe & PCI_PROBE_MMCONF) == 0)
- return 0;
-
if (list_empty(&pci_mmcfg_list))
return 0;

[Hanjun Guo]

Post by Tomasz Nowicki

Post by Boris Ostrovsky

Post by Tomasz Nowicki
+ Stefano

Post by Tomasz Nowicki
In drivers/xen/pci.c, there are arch x86 dependent codes when
CONFIG_PCI_MMCONFIG is enabled, since CONFIG_PCI_MMCONFIG
depends on ACPI, so this will prevent XEN PCI running on other
architectures using ACPI with PCI_MMCONFIG enabled (such as ARM64).
Fortunatly, it can be sloved in a simple way. In drivers/xen/pci.c,
the only x86 dependent code is if ((pci_probe & PCI_PROBE_MMCONF) == 0),
and it's defined in asm/pci_x86.h, the code means that
if the PCI resource is not probed in PCI_PROBE_MMCONF way, just
ingnore the xen mcfg init. Actually this is duplicate, because
if PCI resource is not probed in PCI_PROBE_MMCONF way, the
pci_mmconfig_list will be empty, and the if (list_empty())
after it will do the same job.
So just remove the arch related code and the head file, this
will be no functional change for x86, and also makes xen/pci.c
usable for other architectures.
---
drivers/xen/pci.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
index 6785ebb..9a8dbe3 100644
--- a/drivers/xen/pci.c
+++ b/drivers/xen/pci.c
@@ -28,9 +28,6 @@
#include <asm/xen/hypervisor.h>
#include <asm/xen/hypercall.h>
#include "../pci/pci.h"
-#ifdef CONFIG_PCI_MMCONFIG
-#include <asm/pci_x86.h>
-#endif

Assuming this still compiles on x86 now that this include file is removed

drivers/xen/pci.c:221:18: error: ‘pci_mmcfg_list’ undeclared (first
use in this function)
drivers/xen/pci.c:221:18: note: each undeclared identifier is reported
only once for each f

+#include <linux/ecam.h>
Will fix this in next version, thanks!

Hmm, I think we just missed the head file, but the logic
of this patch is still right.

Thanks
Hanjun

[Boris Ostrovsky]

Post by Hanjun Guo

Post by Tomasz Nowicki

Post by Boris Ostrovsky

Post by Tomasz Nowicki
+ Stefano

Post by Tomasz Nowicki
In drivers/xen/pci.c, there are arch x86 dependent codes when
CONFIG_PCI_MMCONFIG is enabled, since CONFIG_PCI_MMCONFIG
depends on ACPI, so this will prevent XEN PCI running on other
architectures using ACPI with PCI_MMCONFIG enabled (such as ARM64).
Fortunatly, it can be sloved in a simple way. In drivers/xen/pci.c,
the only x86 dependent code is if ((pci_probe & PCI_PROBE_MMCONF) == 0),
and it's defined in asm/pci_x86.h, the code means that
if the PCI resource is not probed in PCI_PROBE_MMCONF way, just
ingnore the xen mcfg init. Actually this is duplicate, because
if PCI resource is not probed in PCI_PROBE_MMCONF way, the
pci_mmconfig_list will be empty, and the if (list_empty())
after it will do the same job.
So just remove the arch related code and the head file, this
will be no functional change for x86, and also makes xen/pci.c
usable for other architectures.
---
drivers/xen/pci.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
index 6785ebb..9a8dbe3 100644
--- a/drivers/xen/pci.c
+++ b/drivers/xen/pci.c
@@ -28,9 +28,6 @@
#include <asm/xen/hypervisor.h>
#include <asm/xen/hypercall.h>
#include "../pci/pci.h"
-#ifdef CONFIG_PCI_MMCONFIG
-#include <asm/pci_x86.h>
-#endif

Assuming this still compiles on x86 now that this include file is removed

drivers/xen/pci.c:221:18: error: ‘pci_mmcfg_list’ undeclared (first
use in this function)
drivers/xen/pci.c:221:18: note: each undeclared identifier is reported
only once for each f

+#include <linux/ecam.h>
Will fix this in next version, thanks!

Hmm, I think we just missed the head file, but the logic
of this patch is still right.

Yes, removing the test should be safe.

-boris

[Tomasz Nowicki]

Because of two patch series:
1. Jiang Liu's common interface to support PCI host bridge init
2. Refactoring of MMCONFIG, part of this patch set
now we can think about PCI buses enumeration for ARM64 and ACPI tables.

This patch introduce ACPI based PCI hostbridge init calls which
use information from MCFG table (PCI config space regions) and
_CRS (IO/irq resources) to initialize PCI hostbridge.

Signed-off-by: Tomasz Nowicki <***@semihalf.com>
Signed-off-by: Hanjun Guo <***@linaro.org>
Signed-off-by: Suravee Suthikulpanit <***@amd.com>
CC: Arnd Bergmann <***@arndb.de>
CC: Catalin Marinas <***@arm.com>
CC: Liviu Dudau <***@arm.com>
CC: Lorenzo Pieralisi <***@arm.com>
CC: Will Deacon <***@arm.com>
---
arch/arm64/Kconfig | 6 ++
arch/arm64/kernel/pci.c | 208 +++++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 202 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 07d1811..bbcc6b1 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -89,6 +89,7 @@ config ARM64
select SPARSE_IRQ
select SYSCTL_EXCEPTION_TRACE
select HAVE_CONTEXT_TRACKING
+ select HAVE_PCI_ECAM
help
ARM 64-bit (AArch64) Linux support.

@@ -202,6 +203,11 @@ source "drivers/pci/Kconfig"
source "drivers/pci/pcie/Kconfig"
source "drivers/pci/hotplug/Kconfig"

+config PCI_MMCONFIG
+ def_bool y
+ select PCI_ECAM
+ depends on ACPI
+
endmenu

menu "Kernel Features"
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index b3d098b..66cc1ae 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -11,12 +11,15 @@
*/

#include <linux/acpi.h>
+#include <linux/ecam.h>
#include <linux/init.h>
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/mm.h>
+#include <linux/of_address.h>
#include <linux/of_pci.h>
#include <linux/of_platform.h>
+#include <linux/pci-acpi.h>
#include <linux/slab.h>

#include <asm/pci-bridge.h>
@@ -52,35 +55,216 @@ 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 from DT/ACPI when adding a new device
*/
int pcibios_add_device(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
+ acpi_pci_irq_enable(dev);
+#endif

return 0;
}

+#ifdef CONFIG_ACPI
+int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
+{
+ struct acpi_pci_root *root = bridge->bus->sysdata;
+
+ ACPI_COMPANION_SET(&bridge->dev, root->device);
+ 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);
+}
+
+static int __init pcibios_assign_resources(void)
+{
+ if (acpi_disabled)
+ return 0;
+
+ pci_assign_unassigned_resources();
+ return 0;
+}
/*
- * raw_pci_read/write - Platform-specific PCI config space access.
+ * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
+ * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
*/
-int raw_pci_read(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 *val)
+rootfs_initcall(pcibios_assign_resources);
+
+static void __iomem *
+pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
+ if (cfg && cfg->virt)
+ return cfg->virt +
+ (PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
+ offset;
+ return NULL;
}

-int raw_pci_write(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 val)
+struct pci_ops pci_root_ops = {
+ .map_bus = pci_mcfg_dev_base,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,
+};
+
+#ifdef CONFIG_PCI_MMCONFIG
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+ struct acpi_pci_root *root;
+ int seg, start, end, err;
+
+ root = ci->root;
+ seg = root->segment;
+ start = root->secondary.start;
+ end = root->secondary.end;
+
+ cfg = pci_mmconfig_lookup(seg, start);
+ if (cfg)
+ return 0;
+
+ cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
+ if (!cfg)
+ return -ENOMEM;
+
+ err = pci_mmconfig_inject(cfg);
+ return err;
}

-#ifdef CONFIG_ACPI
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ struct acpi_pci_root *root = ci->root;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
+ if (cfg)
+ return;
+
+ if (cfg->hot_added)
+ pci_mmconfig_delete(root->segment, root->secondary.start,
+ root->secondary.end);
+}
+#else
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ return 0;
+}
+
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
+#endif
+
+static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
+{
+ return pci_add_mmconfig_region(ci);
+}
+
+static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
+{
+ pci_remove_mmconfig_region(ci);
+ kfree(ci);
+}
+
+static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
+{
+ struct resource_entry *entry, *tmp;
+ int ret;
+
+ ret = acpi_pci_probe_root_resources(ci);
+ if (ret < 0)
+ return ret;
+
+ resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
+ struct resource *res = entry->res;
+
+ /*
+ * Special handling for ARM IO range
+ * TODO: need to move pci_register_io_range() function out
+ * of drivers/of/address.c for both used by DT and ACPI
+ */
+ if (res->flags & IORESOURCE_IO) {
+ unsigned long port;
+ int err;
+ resource_size_t length = res->end - res->start;
+
+ err = pci_register_io_range(res->start, length);
+ if (err) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ port = pci_address_to_pio(res->start);
+ if (port == (unsigned long)-1) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ res->start = port;
+ res->end = res->start + length - 1;
+
+ if (pci_remap_iospace(res, res->start) < 0)
+ resource_list_destroy_entry(entry);
+ }
+ }
+
+ return 0;
+}
+
+static struct acpi_pci_root_ops acpi_pci_root_ops = {
+ .pci_ops = &pci_root_ops,
+ .init_info = pci_acpi_root_init_info,
+ .release_info = pci_acpi_root_release_info,
+ .prepare_resources = pci_acpi_root_prepare_resources,
+};
+
/* 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;
+ int node = acpi_get_node(root->device->handle);
+ int domain = root->segment;
+ int busnum = root->secondary.start;
+ struct acpi_pci_root_info *info;
+ struct pci_bus *bus;
+
+ if (domain && !pci_domains_supported) {
+ pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
+ if (!info) {
+ dev_err(&root->device->dev,
+ "pci_bus %04x:%02x: ignored (out of memory)\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
+
+ /* After the PCI-E bus has been walked and all devices discovered,
+ * configure any settings of the fabric that might be necessary.
+ */
+ if (bus) {
+ struct pci_bus *child;
+
+ list_for_each_entry(child, &bus->children, node)
+ pcie_bus_configure_settings(child);
+ }
+
+ return bus;
}
#endif

[L***@arm.com]

Post by Tomasz Nowicki
1. Jiang Liu's common interface to support PCI host bridge init
2. Refactoring of MMCONFIG, part of this patch set
now we can think about PCI buses enumeration for ARM64 and ACPI tables.
This patch introduce ACPI based PCI hostbridge init calls which
use information from MCFG table (PCI config space regions) and
_CRS (IO/irq resources) to initialize PCI hostbridge.
---
arch/arm64/Kconfig | 6 ++
arch/arm64/kernel/pci.c | 208 +++++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 202 insertions(+), 12 deletions(-)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 07d1811..bbcc6b1 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -89,6 +89,7 @@ config ARM64
select SPARSE_IRQ
select SYSCTL_EXCEPTION_TRACE
select HAVE_CONTEXT_TRACKING
+ select HAVE_PCI_ECAM
help
ARM 64-bit (AArch64) Linux support.
@@ -202,6 +203,11 @@ source "drivers/pci/Kconfig"
source "drivers/pci/pcie/Kconfig"
source "drivers/pci/hotplug/Kconfig"
+config PCI_MMCONFIG
+ def_bool y
+ select PCI_ECAM
+ depends on ACPI
+
endmenu
menu "Kernel Features"
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index b3d098b..66cc1ae 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -11,12 +11,15 @@
*/
#include <linux/acpi.h>
+#include <linux/ecam.h>
#include <linux/init.h>
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/mm.h>
+#include <linux/of_address.h>
#include <linux/of_pci.h>
#include <linux/of_platform.h>
+#include <linux/pci-acpi.h>
#include <linux/slab.h>
#include <asm/pci-bridge.h>
@@ -52,35 +55,216 @@ 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 from DT/ACPI when adding a new device
*/
int pcibios_add_device(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
+ acpi_pci_irq_enable(dev);
+#endif
return 0;
}
+#ifdef CONFIG_ACPI
+int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
+{
+ struct acpi_pci_root *root = bridge->bus->sysdata;
+
+ ACPI_COMPANION_SET(&bridge->dev, root->device);
+ 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);
+}
+
+static int __init pcibios_assign_resources(void)
+{
+ if (acpi_disabled)
+ return 0;
+
+ pci_assign_unassigned_resources();
+ return 0;

You can change this function into:
{
if (!acpi_disabled)
pci_assign_unassigned_resources();

return 0;
}

as the equivalent but shorter form.

Post by Tomasz Nowicki
+}
/*
- * raw_pci_read/write - Platform-specific PCI config space access.
+ * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
+ * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
*/
-int raw_pci_read(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 *val)

What happened with raw_pci_{read,write} ? Why do you remove them?

Post by Tomasz Nowicki
+rootfs_initcall(pcibios_assign_resources);

Would you be so kind and explain to me why you need this initcall?
Can you not set the PCI_REASSIGN_ALL_RSRC flag before calling
pci_scan_root_bus() ?

I haven't focused on ACPI before so I'm a bit hazy on the init order when
that is enabled. That being said, I don't like adding in the architecture
code initcall hooks just to fix up some dependency orders that could / should
be fixed some other way.

Post by Tomasz Nowicki
+
+static void __iomem *
+pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
+ if (cfg && cfg->virt)
+ return cfg->virt +
+ (PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
+ offset;
+ return NULL;
}
-int raw_pci_write(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 val)
+struct pci_ops pci_root_ops = {
+ .map_bus = pci_mcfg_dev_base,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,
+};
+
+#ifdef CONFIG_PCI_MMCONFIG
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+ struct acpi_pci_root *root;
+ int seg, start, end, err;
+
+ root = ci->root;
+ seg = root->segment;
+ start = root->secondary.start;
+ end = root->secondary.end;
+
+ cfg = pci_mmconfig_lookup(seg, start);
+ if (cfg)
+ return 0;
+
+ cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
+ if (!cfg)
+ return -ENOMEM;
+
+ err = pci_mmconfig_inject(cfg);
+ return err;
}
-#ifdef CONFIG_ACPI
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ struct acpi_pci_root *root = ci->root;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
+ if (cfg)
+ return;
+
+ if (cfg->hot_added)
+ pci_mmconfig_delete(root->segment, root->secondary.start,
+ root->secondary.end);
+}
+#else
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ return 0;
+}
+
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
+#endif
+
+static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
+{
+ return pci_add_mmconfig_region(ci);
+}
+
+static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
+{
+ pci_remove_mmconfig_region(ci);
+ kfree(ci);
+}
+
+static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
+{
+ struct resource_entry *entry, *tmp;
+ int ret;
+
+ ret = acpi_pci_probe_root_resources(ci);
+ if (ret < 0)
+ return ret;
+
+ resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
+ struct resource *res = entry->res;
+
+ /*
+ * Special handling for ARM IO range

There is nothing ARM specific here. It should apply to any memory mapped IO range.

Post by Tomasz Nowicki
+ * TODO: need to move pci_register_io_range() function out
+ * of drivers/of/address.c for both used by DT and ACPI
+ */
+ if (res->flags & IORESOURCE_IO) {
+ unsigned long port;
+ int err;
+ resource_size_t length = res->end - res->start;
+
+ err = pci_register_io_range(res->start, length);
+ if (err) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ port = pci_address_to_pio(res->start);
+ if (port == (unsigned long)-1) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ res->start = port;
+ res->end = res->start + length - 1;
+
+ if (pci_remap_iospace(res, res->start) < 0)
+ resource_list_destroy_entry(entry);
+ }
+ }
+
+ return 0;
+}
+
+static struct acpi_pci_root_ops acpi_pci_root_ops = {
+ .pci_ops = &pci_root_ops,
+ .init_info = pci_acpi_root_init_info,
+ .release_info = pci_acpi_root_release_info,
+ .prepare_resources = pci_acpi_root_prepare_resources,
+};
+
/* 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;
+ int node = acpi_get_node(root->device->handle);
+ int domain = root->segment;
+ int busnum = root->secondary.start;
+ struct acpi_pci_root_info *info;
+ struct pci_bus *bus;
+
+ if (domain && !pci_domains_supported) {
+ pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
+ if (!info) {
+ dev_err(&root->device->dev,
+ "pci_bus %04x:%02x: ignored (out of memory)\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
+
+ /* After the PCI-E bus has been walked and all devices discovered,
+ * configure any settings of the fabric that might be necessary.
+ */
+ if (bus) {
+ struct pci_bus *child;
+
+ list_for_each_entry(child, &bus->children, node)
+ pcie_bus_configure_settings(child);
+ }
+
+ return bus;
}
#endif
--
1.9.1

[Tomasz Nowicki]

Post by L***@arm.com

Post by Tomasz Nowicki
1. Jiang Liu's common interface to support PCI host bridge init
2. Refactoring of MMCONFIG, part of this patch set
now we can think about PCI buses enumeration for ARM64 and ACPI tables.
This patch introduce ACPI based PCI hostbridge init calls which
use information from MCFG table (PCI config space regions) and
_CRS (IO/irq resources) to initialize PCI hostbridge.
---
arch/arm64/Kconfig | 6 ++
arch/arm64/kernel/pci.c | 208 +++++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 202 insertions(+), 12 deletions(-)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 07d1811..bbcc6b1 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -89,6 +89,7 @@ config ARM64
select SPARSE_IRQ
select SYSCTL_EXCEPTION_TRACE
select HAVE_CONTEXT_TRACKING
+ select HAVE_PCI_ECAM
help
ARM 64-bit (AArch64) Linux support.
@@ -202,6 +203,11 @@ source "drivers/pci/Kconfig"
source "drivers/pci/pcie/Kconfig"
source "drivers/pci/hotplug/Kconfig"
+config PCI_MMCONFIG
+ def_bool y
+ select PCI_ECAM
+ depends on ACPI
+
endmenu
menu "Kernel Features"
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index b3d098b..66cc1ae 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -11,12 +11,15 @@
*/
#include <linux/acpi.h>
+#include <linux/ecam.h>
#include <linux/init.h>
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/mm.h>
+#include <linux/of_address.h>
#include <linux/of_pci.h>
#include <linux/of_platform.h>
+#include <linux/pci-acpi.h>
#include <linux/slab.h>
#include <asm/pci-bridge.h>
@@ -52,35 +55,216 @@ 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 from DT/ACPI when adding a new device
*/
int pcibios_add_device(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
+ acpi_pci_irq_enable(dev);
+#endif
return 0;
}
+#ifdef CONFIG_ACPI
+int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
+{
+ struct acpi_pci_root *root = bridge->bus->sysdata;
+
+ ACPI_COMPANION_SET(&bridge->dev, root->device);
+ 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);
+}
+
+static int __init pcibios_assign_resources(void)
+{
+ if (acpi_disabled)
+ return 0;
+
+ pci_assign_unassigned_resources();
+ return 0;

{
if (!acpi_disabled)
pci_assign_unassigned_resources();
return 0;
}
as the equivalent but shorter form.

Sure, will do.

Post by L***@arm.com

Post by Tomasz Nowicki
+}
/*
- * raw_pci_read/write - Platform-specific PCI config space access.
+ * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
+ * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
*/
-int raw_pci_read(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 *val)

What happened with raw_pci_{read,write} ? Why do you remove them?

We do not need raw_pci_{read,write} any more, we will use empty
raw_pci_{read,write} from mcfg.c introduced in patch 6/11.

I think this is another candidate to split up, first I will remove these
raw_pci_{read,write} accessors and then introduce ACPI PCI hostbridge
init, it will be easier to review, what do you think?

Post by L***@arm.com

Post by Tomasz Nowicki
+rootfs_initcall(pcibios_assign_resources);

Would you be so kind and explain to me why you need this initcall?
Can you not set the PCI_REASSIGN_ALL_RSRC flag before calling
pci_scan_root_bus() ?
I haven't focused on ACPI before so I'm a bit hazy on the init order when
that is enabled. That being said, I don't like adding in the architecture
code initcall hooks just to fix up some dependency orders that could / should
be fixed some other way.

My idea was to defer resource reassigning to give a chance for running
DECLARE_PCI_FIXUP_FINAL. I used DECLARE_PCI_FIXUP_FINAL to set
IORESOURCE_PCI_FIXED for some PCI devices. Now I am not sure we need to
defer it, my DECLARE_PCI_FIXUP_FINALs might be done in firmware, let me
get back to you on this.

Post by L***@arm.com

Post by Tomasz Nowicki
+
+static void __iomem *
+pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
+ if (cfg && cfg->virt)
+ return cfg->virt +
+ (PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
+ offset;
+ return NULL;
}
-int raw_pci_write(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 val)
+struct pci_ops pci_root_ops = {
+ .map_bus = pci_mcfg_dev_base,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,
+};
+
+#ifdef CONFIG_PCI_MMCONFIG
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+ struct acpi_pci_root *root;
+ int seg, start, end, err;
+
+ root = ci->root;
+ seg = root->segment;
+ start = root->secondary.start;
+ end = root->secondary.end;
+
+ cfg = pci_mmconfig_lookup(seg, start);
+ if (cfg)
+ return 0;
+
+ cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
+ if (!cfg)
+ return -ENOMEM;
+
+ err = pci_mmconfig_inject(cfg);
+ return err;
}
-#ifdef CONFIG_ACPI
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ struct acpi_pci_root *root = ci->root;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
+ if (cfg)
+ return;
+
+ if (cfg->hot_added)
+ pci_mmconfig_delete(root->segment, root->secondary.start,
+ root->secondary.end);
+}
+#else
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ return 0;
+}
+
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
+#endif
+
+static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
+{
+ return pci_add_mmconfig_region(ci);
+}
+
+static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
+{
+ pci_remove_mmconfig_region(ci);
+ kfree(ci);
+}
+
+static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
+{
+ struct resource_entry *entry, *tmp;
+ int ret;
+
+ ret = acpi_pci_probe_root_resources(ci);
+ if (ret < 0)
+ return ret;
+
+ resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
+ struct resource *res = entry->res;
+
+ /*
+ * Special handling for ARM IO range

There is nothing ARM specific here. It should apply to any memory mapped IO range.

OK, I will remove that comment.

Post by L***@arm.com

Post by Tomasz Nowicki
+ * TODO: need to move pci_register_io_range() function out
+ * of drivers/of/address.c for both used by DT and ACPI
+ */
+ if (res->flags & IORESOURCE_IO) {
+ unsigned long port;
+ int err;
+ resource_size_t length = res->end - res->start;
+
+ err = pci_register_io_range(res->start, length);
+ if (err) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ port = pci_address_to_pio(res->start);
+ if (port == (unsigned long)-1) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ res->start = port;
+ res->end = res->start + length - 1;
+
+ if (pci_remap_iospace(res, res->start) < 0)
+ resource_list_destroy_entry(entry);
+ }
+ }
+
+ return 0;
+}
+
+static struct acpi_pci_root_ops acpi_pci_root_ops = {
+ .pci_ops = &pci_root_ops,
+ .init_info = pci_acpi_root_init_info,
+ .release_info = pci_acpi_root_release_info,
+ .prepare_resources = pci_acpi_root_prepare_resources,
+};
+
/* 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;
+ int node = acpi_get_node(root->device->handle);
+ int domain = root->segment;
+ int busnum = root->secondary.start;
+ struct acpi_pci_root_info *info;
+ struct pci_bus *bus;
+
+ if (domain && !pci_domains_supported) {
+ pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
+ if (!info) {
+ dev_err(&root->device->dev,
+ "pci_bus %04x:%02x: ignored (out of memory)\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
+
+ /* After the PCI-E bus has been walked and all devices discovered,
+ * configure any settings of the fabric that might be necessary.
+ */
+ if (bus) {
+ struct pci_bus *child;
+
+ list_for_each_entry(child, &bus->children, node)
+ pcie_bus_configure_settings(child);
+ }
+
+ return bus;
}
#endif
--
1.9.1

[L***@arm.com]

Post by Tomasz Nowicki

Post by L***@arm.com

Post by Tomasz Nowicki
1. Jiang Liu's common interface to support PCI host bridge init
2. Refactoring of MMCONFIG, part of this patch set
now we can think about PCI buses enumeration for ARM64 and ACPI tables.
This patch introduce ACPI based PCI hostbridge init calls which
use information from MCFG table (PCI config space regions) and
_CRS (IO/irq resources) to initialize PCI hostbridge.
---
arch/arm64/Kconfig | 6 ++
arch/arm64/kernel/pci.c | 208 +++++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 202 insertions(+), 12 deletions(-)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 07d1811..bbcc6b1 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -89,6 +89,7 @@ config ARM64
select SPARSE_IRQ
select SYSCTL_EXCEPTION_TRACE
select HAVE_CONTEXT_TRACKING
+ select HAVE_PCI_ECAM
help
ARM 64-bit (AArch64) Linux support.
@@ -202,6 +203,11 @@ source "drivers/pci/Kconfig"
source "drivers/pci/pcie/Kconfig"
source "drivers/pci/hotplug/Kconfig"
+config PCI_MMCONFIG
+ def_bool y
+ select PCI_ECAM
+ depends on ACPI
+
endmenu
menu "Kernel Features"
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index b3d098b..66cc1ae 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -11,12 +11,15 @@
*/
#include <linux/acpi.h>
+#include <linux/ecam.h>
#include <linux/init.h>
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/mm.h>
+#include <linux/of_address.h>
#include <linux/of_pci.h>
#include <linux/of_platform.h>
+#include <linux/pci-acpi.h>
#include <linux/slab.h>
#include <asm/pci-bridge.h>
@@ -52,35 +55,216 @@ 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 from DT/ACPI when adding a new device
*/
int pcibios_add_device(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
+ acpi_pci_irq_enable(dev);
+#endif
return 0;
}
+#ifdef CONFIG_ACPI
+int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
+{
+ struct acpi_pci_root *root = bridge->bus->sysdata;
+
+ ACPI_COMPANION_SET(&bridge->dev, root->device);
+ 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);
+}
+
+static int __init pcibios_assign_resources(void)
+{
+ if (acpi_disabled)
+ return 0;
+
+ pci_assign_unassigned_resources();
+ return 0;

{
if (!acpi_disabled)
pci_assign_unassigned_resources();
return 0;
}
as the equivalent but shorter form.

Sure, will do.

Post by L***@arm.com

Post by Tomasz Nowicki
+}
/*
- * raw_pci_read/write - Platform-specific PCI config space access.
+ * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
+ * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
*/
-int raw_pci_read(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 *val)

What happened with raw_pci_{read,write} ? Why do you remove them?

We do not need raw_pci_{read,write} any more, we will use empty
raw_pci_{read,write} from mcfg.c introduced in patch 6/11.
I think this is another candidate to split up, first I will remove these
raw_pci_{read,write} accessors and then introduce ACPI PCI hostbridge init,
it will be easier to review, what do you think?

Yes, I like that. If the calls were redundant after 6/11 then they should
not have been kept until this patch to be removed.

Post by Tomasz Nowicki

Post by L***@arm.com

Post by Tomasz Nowicki
+rootfs_initcall(pcibios_assign_resources);

Would you be so kind and explain to me why you need this initcall?
Can you not set the PCI_REASSIGN_ALL_RSRC flag before calling
pci_scan_root_bus() ?
I haven't focused on ACPI before so I'm a bit hazy on the init order when
that is enabled. That being said, I don't like adding in the architecture
code initcall hooks just to fix up some dependency orders that could / should
be fixed some other way.

My idea was to defer resource reassigning to give a chance for running
DECLARE_PCI_FIXUP_FINAL. I used DECLARE_PCI_FIXUP_FINAL to set
IORESOURCE_PCI_FIXED for some PCI devices. Now I am not sure we need to
defer it, my DECLARE_PCI_FIXUP_FINALs might be done in firmware, let me get
back to you on this.

If fixups can be done in firmware then that is the preferred direction nowadays.

Post by Tomasz Nowicki

Post by L***@arm.com

Post by Tomasz Nowicki
+
+static void __iomem *
+pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
+ if (cfg && cfg->virt)
+ return cfg->virt +
+ (PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
+ offset;
+ return NULL;
}
-int raw_pci_write(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 val)
+struct pci_ops pci_root_ops = {
+ .map_bus = pci_mcfg_dev_base,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,
+};
+
+#ifdef CONFIG_PCI_MMCONFIG
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+ struct acpi_pci_root *root;
+ int seg, start, end, err;
+
+ root = ci->root;
+ seg = root->segment;
+ start = root->secondary.start;
+ end = root->secondary.end;
+
+ cfg = pci_mmconfig_lookup(seg, start);
+ if (cfg)
+ return 0;
+
+ cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
+ if (!cfg)
+ return -ENOMEM;
+
+ err = pci_mmconfig_inject(cfg);
+ return err;
}
-#ifdef CONFIG_ACPI
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ struct acpi_pci_root *root = ci->root;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
+ if (cfg)
+ return;
+
+ if (cfg->hot_added)
+ pci_mmconfig_delete(root->segment, root->secondary.start,
+ root->secondary.end);
+}
+#else
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ return 0;
+}
+
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
+#endif
+
+static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
+{
+ return pci_add_mmconfig_region(ci);
+}
+
+static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
+{
+ pci_remove_mmconfig_region(ci);
+ kfree(ci);
+}
+
+static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
+{
+ struct resource_entry *entry, *tmp;
+ int ret;
+
+ ret = acpi_pci_probe_root_resources(ci);
+ if (ret < 0)
+ return ret;
+
+ resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
+ struct resource *res = entry->res;
+
+ /*
+ * Special handling for ARM IO range

There is nothing ARM specific here. It should apply to any memory mapped IO range.

OK, I will remove that comment.

Thanks!

Liviu

Post by Tomasz Nowicki

Post by L***@arm.com

Post by Tomasz Nowicki
+ * TODO: need to move pci_register_io_range() function out
+ * of drivers/of/address.c for both used by DT and ACPI
+ */
+ if (res->flags & IORESOURCE_IO) {
+ unsigned long port;
+ int err;
+ resource_size_t length = res->end - res->start;
+
+ err = pci_register_io_range(res->start, length);
+ if (err) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ port = pci_address_to_pio(res->start);
+ if (port == (unsigned long)-1) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ res->start = port;
+ res->end = res->start + length - 1;
+
+ if (pci_remap_iospace(res, res->start) < 0)
+ resource_list_destroy_entry(entry);
+ }
+ }
+
+ return 0;
+}
+
+static struct acpi_pci_root_ops acpi_pci_root_ops = {
+ .pci_ops = &pci_root_ops,
+ .init_info = pci_acpi_root_init_info,
+ .release_info = pci_acpi_root_release_info,
+ .prepare_resources = pci_acpi_root_prepare_resources,
+};
+
/* 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;
+ int node = acpi_get_node(root->device->handle);
+ int domain = root->segment;
+ int busnum = root->secondary.start;
+ struct acpi_pci_root_info *info;
+ struct pci_bus *bus;
+
+ if (domain && !pci_domains_supported) {
+ pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
+ if (!info) {
+ dev_err(&root->device->dev,
+ "pci_bus %04x:%02x: ignored (out of memory)\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
+
+ /* After the PCI-E bus has been walked and all devices discovered,
+ * configure any settings of the fabric that might be necessary.
+ */
+ if (bus) {
+ struct pci_bus *child;
+
+ list_for_each_entry(child, &bus->children, node)
+ pcie_bus_configure_settings(child);
+ }
+
+ return bus;
}
#endif
--
1.9.1

[Lorenzo Pieralisi]

[...]

Post by L***@arm.com

Post by Tomasz Nowicki
+static int __init pcibios_assign_resources(void)
+{
+ if (acpi_disabled)
+ return 0;
+
+ pci_assign_unassigned_resources();
+ return 0;

{
if (!acpi_disabled)
pci_assign_unassigned_resources();
return 0;
}
as the equivalent but shorter form.

I do not think it is a matter of code style here, it is a matter
of understanding when and if we want to reassign resources on ACPI
systems, it is an open question on ARM64 that must be sorted out (ie we
ignore FW/BARs set-up entirely).

Post by L***@arm.com

Post by Tomasz Nowicki
+}
/*
- * raw_pci_read/write - Platform-specific PCI config space access.
+ * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
+ * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
*/
-int raw_pci_read(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 *val)

What happened with raw_pci_{read,write} ? Why do you remove them?

Post by Tomasz Nowicki
+rootfs_initcall(pcibios_assign_resources);

Would you be so kind and explain to me why you need this initcall?
Can you not set the PCI_REASSIGN_ALL_RSRC flag before calling
pci_scan_root_bus() ?

On what basis ? BTW, PCI core code does not assign unassigned resources
anyway even if that flag is set, so some policy has to be defined here.

Thanks,
Lorenzo

Post by L***@arm.com
I haven't focused on ACPI before so I'm a bit hazy on the init order when
that is enabled. That being said, I don't like adding in the architecture
code initcall hooks just to fix up some dependency orders that could / should
be fixed some other way.

Post by Tomasz Nowicki
+
+static void __iomem *
+pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
+ if (cfg && cfg->virt)
+ return cfg->virt +
+ (PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
+ offset;
+ return NULL;
}
-int raw_pci_write(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 val)
+struct pci_ops pci_root_ops = {
+ .map_bus = pci_mcfg_dev_base,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,
+};
+
+#ifdef CONFIG_PCI_MMCONFIG
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+ struct acpi_pci_root *root;
+ int seg, start, end, err;
+
+ root = ci->root;
+ seg = root->segment;
+ start = root->secondary.start;
+ end = root->secondary.end;
+
+ cfg = pci_mmconfig_lookup(seg, start);
+ if (cfg)
+ return 0;
+
+ cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
+ if (!cfg)
+ return -ENOMEM;
+
+ err = pci_mmconfig_inject(cfg);
+ return err;
}
-#ifdef CONFIG_ACPI
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ struct acpi_pci_root *root = ci->root;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
+ if (cfg)
+ return;
+
+ if (cfg->hot_added)
+ pci_mmconfig_delete(root->segment, root->secondary.start,
+ root->secondary.end);
+}
+#else
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ return 0;
+}
+
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
+#endif
+
+static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
+{
+ return pci_add_mmconfig_region(ci);
+}
+
+static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
+{
+ pci_remove_mmconfig_region(ci);
+ kfree(ci);
+}
+
+static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
+{
+ struct resource_entry *entry, *tmp;
+ int ret;
+
+ ret = acpi_pci_probe_root_resources(ci);
+ if (ret < 0)
+ return ret;
+
+ resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
+ struct resource *res = entry->res;
+
+ /*
+ * Special handling for ARM IO range

There is nothing ARM specific here. It should apply to any memory mapped IO range.

Post by Tomasz Nowicki
+ * TODO: need to move pci_register_io_range() function out
+ * of drivers/of/address.c for both used by DT and ACPI
+ */
+ if (res->flags & IORESOURCE_IO) {
+ unsigned long port;
+ int err;
+ resource_size_t length = res->end - res->start;
+
+ err = pci_register_io_range(res->start, length);
+ if (err) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ port = pci_address_to_pio(res->start);
+ if (port == (unsigned long)-1) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ res->start = port;
+ res->end = res->start + length - 1;
+
+ if (pci_remap_iospace(res, res->start) < 0)
+ resource_list_destroy_entry(entry);
+ }
+ }
+
+ return 0;
+}
+
+static struct acpi_pci_root_ops acpi_pci_root_ops = {
+ .pci_ops = &pci_root_ops,
+ .init_info = pci_acpi_root_init_info,
+ .release_info = pci_acpi_root_release_info,
+ .prepare_resources = pci_acpi_root_prepare_resources,
+};
+
/* 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;
+ int node = acpi_get_node(root->device->handle);
+ int domain = root->segment;
+ int busnum = root->secondary.start;
+ struct acpi_pci_root_info *info;
+ struct pci_bus *bus;
+
+ if (domain && !pci_domains_supported) {
+ pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
+ if (!info) {
+ dev_err(&root->device->dev,
+ "pci_bus %04x:%02x: ignored (out of memory)\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
+
+ /* After the PCI-E bus has been walked and all devices discovered,
+ * configure any settings of the fabric that might be necessary.
+ */
+ if (bus) {
+ struct pci_bus *child;
+
+ list_for_each_entry(child, &bus->children, node)
+ pcie_bus_configure_settings(child);
+ }
+
+ return bus;
}
#endif
--
1.9.1

--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯

[L***@arm.com]

Post by Lorenzo Pieralisi
[...]

Post by L***@arm.com

Post by Tomasz Nowicki
+static int __init pcibios_assign_resources(void)
+{
+ if (acpi_disabled)
+ return 0;
+
+ pci_assign_unassigned_resources();
+ return 0;

{
if (!acpi_disabled)
pci_assign_unassigned_resources();
return 0;
}
as the equivalent but shorter form.

I do not think it is a matter of code style here, it is a matter
of understanding when and if we want to reassign resources on ACPI
systems, it is an open question on ARM64 that must be sorted out (ie we
ignore FW/BARs set-up entirely).

I was reviewing the code here, not doing any technical guidance, I'm
leaving that to you as you are more involved around ACPI.

Post by Lorenzo Pieralisi

Post by L***@arm.com

Post by Tomasz Nowicki
+}
/*
- * raw_pci_read/write - Platform-specific PCI config space access.
+ * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
+ * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
*/
-int raw_pci_read(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 *val)

What happened with raw_pci_{read,write} ? Why do you remove them?

Post by Tomasz Nowicki
+rootfs_initcall(pcibios_assign_resources);

Would you be so kind and explain to me why you need this initcall?
Can you not set the PCI_REASSIGN_ALL_RSRC flag before calling
pci_scan_root_bus() ?

On what basis ? BTW, PCI core code does not assign unassigned resources
anyway even if that flag is set, so some policy has to be defined here.

I was thinking that ACPI code can do that if they seem to depend on the resources
being assigned during root bus scan. I was not implying that PCI core code enforces
that.

Best regards,
Liviu

Post by Lorenzo Pieralisi
Thanks,
Lorenzo

Post by L***@arm.com
I haven't focused on ACPI before so I'm a bit hazy on the init order when
that is enabled. That being said, I don't like adding in the architecture
code initcall hooks just to fix up some dependency orders that could / should
be fixed some other way.

Post by Tomasz Nowicki
+
+static void __iomem *
+pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
+ if (cfg && cfg->virt)
+ return cfg->virt +
+ (PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
+ offset;
+ return NULL;
}
-int raw_pci_write(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 val)
+struct pci_ops pci_root_ops = {
+ .map_bus = pci_mcfg_dev_base,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,
+};
+
+#ifdef CONFIG_PCI_MMCONFIG
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+ struct acpi_pci_root *root;
+ int seg, start, end, err;
+
+ root = ci->root;
+ seg = root->segment;
+ start = root->secondary.start;
+ end = root->secondary.end;
+
+ cfg = pci_mmconfig_lookup(seg, start);
+ if (cfg)
+ return 0;
+
+ cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
+ if (!cfg)
+ return -ENOMEM;
+
+ err = pci_mmconfig_inject(cfg);
+ return err;
}
-#ifdef CONFIG_ACPI
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ struct acpi_pci_root *root = ci->root;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
+ if (cfg)
+ return;
+
+ if (cfg->hot_added)
+ pci_mmconfig_delete(root->segment, root->secondary.start,
+ root->secondary.end);
+}
+#else
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ return 0;
+}
+
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
+#endif
+
+static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
+{
+ return pci_add_mmconfig_region(ci);
+}
+
+static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
+{
+ pci_remove_mmconfig_region(ci);
+ kfree(ci);
+}
+
+static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
+{
+ struct resource_entry *entry, *tmp;
+ int ret;
+
+ ret = acpi_pci_probe_root_resources(ci);
+ if (ret < 0)
+ return ret;
+
+ resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
+ struct resource *res = entry->res;
+
+ /*
+ * Special handling for ARM IO range

There is nothing ARM specific here. It should apply to any memory mapped IO range.

Post by Tomasz Nowicki
+ * TODO: need to move pci_register_io_range() function out
+ * of drivers/of/address.c for both used by DT and ACPI
+ */
+ if (res->flags & IORESOURCE_IO) {
+ unsigned long port;
+ int err;
+ resource_size_t length = res->end - res->start;
+
+ err = pci_register_io_range(res->start, length);
+ if (err) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ port = pci_address_to_pio(res->start);
+ if (port == (unsigned long)-1) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ res->start = port;
+ res->end = res->start + length - 1;
+
+ if (pci_remap_iospace(res, res->start) < 0)
+ resource_list_destroy_entry(entry);
+ }
+ }
+
+ return 0;
+}
+
+static struct acpi_pci_root_ops acpi_pci_root_ops = {
+ .pci_ops = &pci_root_ops,
+ .init_info = pci_acpi_root_init_info,
+ .release_info = pci_acpi_root_release_info,
+ .prepare_resources = pci_acpi_root_prepare_resources,
+};
+
/* 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;
+ int node = acpi_get_node(root->device->handle);
+ int domain = root->segment;
+ int busnum = root->secondary.start;
+ struct acpi_pci_root_info *info;
+ struct pci_bus *bus;
+
+ if (domain && !pci_domains_supported) {
+ pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
+ if (!info) {
+ dev_err(&root->device->dev,
+ "pci_bus %04x:%02x: ignored (out of memory)\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
+
+ /* After the PCI-E bus has been walked and all devices discovered,
+ * configure any settings of the fabric that might be necessary.
+ */
+ if (bus) {
+ struct pci_bus *child;
+
+ list_for_each_entry(child, &bus->children, node)
+ pcie_bus_configure_settings(child);
+ }
+
+ return bus;
}
#endif
--
1.9.1

--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Sinan Kaya]

Post by Tomasz Nowicki
1. Jiang Liu's common interface to support PCI host bridge init
2. Refactoring of MMCONFIG, part of this patch set
now we can think about PCI buses enumeration for ARM64 and ACPI tables.
This patch introduce ACPI based PCI hostbridge init calls which
use information from MCFG table (PCI config space regions) and
_CRS (IO/irq resources) to initialize PCI hostbridge.
---
arch/arm64/Kconfig | 6 ++
arch/arm64/kernel/pci.c | 208 +++++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 202 insertions(+), 12 deletions(-)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 07d1811..bbcc6b1 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -89,6 +89,7 @@ config ARM64
select SPARSE_IRQ
select SYSCTL_EXCEPTION_TRACE
select HAVE_CONTEXT_TRACKING
+ select HAVE_PCI_ECAM
help
ARM 64-bit (AArch64) Linux support.
@@ -202,6 +203,11 @@ source "drivers/pci/Kconfig"
source "drivers/pci/pcie/Kconfig"
source "drivers/pci/hotplug/Kconfig"
+config PCI_MMCONFIG
+ def_bool y
+ select PCI_ECAM
+ depends on ACPI
+
endmenu
menu "Kernel Features"
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index b3d098b..66cc1ae 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -11,12 +11,15 @@
*/
#include <linux/acpi.h>
+#include <linux/ecam.h>
#include <linux/init.h>
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/mm.h>
+#include <linux/of_address.h>
#include <linux/of_pci.h>
#include <linux/of_platform.h>
+#include <linux/pci-acpi.h>
#include <linux/slab.h>
#include <asm/pci-bridge.h>
@@ -52,35 +55,216 @@ 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 from DT/ACPI when adding a new device
*/
int pcibios_add_device(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
+ acpi_pci_irq_enable(dev);
+#endif
return 0;
}
+#ifdef CONFIG_ACPI
+int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
+{
+ struct acpi_pci_root *root = bridge->bus->sysdata;
+
+ ACPI_COMPANION_SET(&bridge->dev, root->device);
+ 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);
+}
+
+static int __init pcibios_assign_resources(void)
+{
+ if (acpi_disabled)
+ return 0;
+
+ pci_assign_unassigned_resources();
+ return 0;
+}
/*
- * raw_pci_read/write - Platform-specific PCI config space access.
+ * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
+ * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
*/
-int raw_pci_read(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 *val)
+rootfs_initcall(pcibios_assign_resources);
+
+static void __iomem *
+pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
+ if (cfg && cfg->virt)
+ return cfg->virt +
+ (PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
+ offset;
+ return NULL;
}
-int raw_pci_write(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 val)
+struct pci_ops pci_root_ops = {
+ .map_bus = pci_mcfg_dev_base,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,

Can you change these with pci_generic_config_read32 and
pci_generic_config_write32? We have some targets that can only do 32
bits PCI config space access.

Post by Tomasz Nowicki
+};
+
+#ifdef CONFIG_PCI_MMCONFIG
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+ struct acpi_pci_root *root;
+ int seg, start, end, err;
+
+ root = ci->root;
+ seg = root->segment;
+ start = root->secondary.start;
+ end = root->secondary.end;
+
+ cfg = pci_mmconfig_lookup(seg, start);
+ if (cfg)
+ return 0;
+
+ cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
+ if (!cfg)
+ return -ENOMEM;
+
+ err = pci_mmconfig_inject(cfg);
+ return err;
}
-#ifdef CONFIG_ACPI
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ struct acpi_pci_root *root = ci->root;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
+ if (cfg)
+ return;
+
+ if (cfg->hot_added)
+ pci_mmconfig_delete(root->segment, root->secondary.start,
+ root->secondary.end);
+}
+#else
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ return 0;
+}
+
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
+#endif
+
+static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
+{
+ return pci_add_mmconfig_region(ci);
+}
+
+static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
+{
+ pci_remove_mmconfig_region(ci);
+ kfree(ci);
+}
+
+static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
+{
+ struct resource_entry *entry, *tmp;
+ int ret;
+
+ ret = acpi_pci_probe_root_resources(ci);
+ if (ret < 0)
+ return ret;
+
+ resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
+ struct resource *res = entry->res;
+
+ /*
+ * Special handling for ARM IO range
+ * TODO: need to move pci_register_io_range() function out
+ * of drivers/of/address.c for both used by DT and ACPI
+ */
+ if (res->flags & IORESOURCE_IO) {
+ unsigned long port;
+ int err;
+ resource_size_t length = res->end - res->start;
+
+ err = pci_register_io_range(res->start, length);
+ if (err) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ port = pci_address_to_pio(res->start);
+ if (port == (unsigned long)-1) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ res->start = port;
+ res->end = res->start + length - 1;
+
+ if (pci_remap_iospace(res, res->start) < 0)
+ resource_list_destroy_entry(entry);
+ }
+ }
+
+ return 0;
+}
+
+static struct acpi_pci_root_ops acpi_pci_root_ops = {
+ .pci_ops = &pci_root_ops,
+ .init_info = pci_acpi_root_init_info,
+ .release_info = pci_acpi_root_release_info,
+ .prepare_resources = pci_acpi_root_prepare_resources,
+};
+
/* 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;
+ int node = acpi_get_node(root->device->handle);
+ int domain = root->segment;
+ int busnum = root->secondary.start;
+ struct acpi_pci_root_info *info;
+ struct pci_bus *bus;
+
+ if (domain && !pci_domains_supported) {
+ pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
+ if (!info) {
+ dev_err(&root->device->dev,
+ "pci_bus %04x:%02x: ignored (out of memory)\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
+
+ /* After the PCI-E bus has been walked and all devices discovered,
+ * configure any settings of the fabric that might be necessary.
+ */
+ if (bus) {
+ struct pci_bus *child;
+
+ list_for_each_entry(child, &bus->children, node)
+ pcie_bus_configure_settings(child);
+ }
+
+ return bus;
}
#endif

[Sinan Kaya]

Post by Sinan Kaya

Post by Tomasz Nowicki
1. Jiang Liu's common interface to support PCI host bridge init
2. Refactoring of MMCONFIG, part of this patch set
now we can think about PCI buses enumeration for ARM64 and ACPI tables.
This patch introduce ACPI based PCI hostbridge init calls which
use information from MCFG table (PCI config space regions) and
_CRS (IO/irq resources) to initialize PCI hostbridge.
---
arch/arm64/Kconfig | 6 ++
arch/arm64/kernel/pci.c | 208
+++++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 202 insertions(+), 12 deletions(-)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 07d1811..bbcc6b1 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -89,6 +89,7 @@ config ARM64
select SPARSE_IRQ
select SYSCTL_EXCEPTION_TRACE
select HAVE_CONTEXT_TRACKING
+ select HAVE_PCI_ECAM
help
ARM 64-bit (AArch64) Linux support.
@@ -202,6 +203,11 @@ source "drivers/pci/Kconfig"
source "drivers/pci/pcie/Kconfig"
source "drivers/pci/hotplug/Kconfig"
+config PCI_MMCONFIG
+ def_bool y
+ select PCI_ECAM
+ depends on ACPI
+
endmenu
menu "Kernel Features"
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index b3d098b..66cc1ae 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -11,12 +11,15 @@
*/
#include <linux/acpi.h>
+#include <linux/ecam.h>
#include <linux/init.h>
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/mm.h>
+#include <linux/of_address.h>
#include <linux/of_pci.h>
#include <linux/of_platform.h>
+#include <linux/pci-acpi.h>
#include <linux/slab.h>
#include <asm/pci-bridge.h>
@@ -52,35 +55,216 @@ 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 from DT/ACPI when adding a new device
*/
int pcibios_add_device(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
+ acpi_pci_irq_enable(dev);
+#endif
return 0;
}
+#ifdef CONFIG_ACPI
+int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
+{
+ struct acpi_pci_root *root = bridge->bus->sysdata;
+
+ ACPI_COMPANION_SET(&bridge->dev, root->device);
+ 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);
+}
+
+static int __init pcibios_assign_resources(void)
+{
+ if (acpi_disabled)
+ return 0;
+
+ pci_assign_unassigned_resources();
+ return 0;
+}
/*
- * raw_pci_read/write - Platform-specific PCI config space access.
+ * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
+ * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
*/
-int raw_pci_read(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 *val)
+rootfs_initcall(pcibios_assign_resources);
+
+static void __iomem *
+pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
+ if (cfg && cfg->virt)
+ return cfg->virt +
+ (PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
+ offset;
+ return NULL;
}
-int raw_pci_write(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 val)
+struct pci_ops pci_root_ops = {
+ .map_bus = pci_mcfg_dev_base,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,

Can you change these with pci_generic_config_read32 and
pci_generic_config_write32? We have some targets that can only do 32
bits PCI config space access.

Post by Tomasz Nowicki
+};
+
+#ifdef CONFIG_PCI_MMCONFIG
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+ struct acpi_pci_root *root;
+ int seg, start, end, err;
+
+ root = ci->root;
+ seg = root->segment;
+ start = root->secondary.start;
+ end = root->secondary.end;
+
+ cfg = pci_mmconfig_lookup(seg, start);
+ if (cfg)
+ return 0;
+
+ cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
+ if (!cfg)
+ return -ENOMEM;
+
+ err = pci_mmconfig_inject(cfg);
+ return err;
}
-#ifdef CONFIG_ACPI
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ struct acpi_pci_root *root = ci->root;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
+ if (cfg)
+ return;
+
+ if (cfg->hot_added)
+ pci_mmconfig_delete(root->segment, root->secondary.start,
+ root->secondary.end);
+}
+#else
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ return 0;
+}
+
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
+#endif
+
+static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
+{
+ return pci_add_mmconfig_region(ci);
+}
+
+static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
+{
+ pci_remove_mmconfig_region(ci);
+ kfree(ci);
+}
+
+static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
+{
+ struct resource_entry *entry, *tmp;
+ int ret;
+
+ ret = acpi_pci_probe_root_resources(ci);
+ if (ret < 0)
+ return ret;
+
+ resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
+ struct resource *res = entry->res;
+
+ /*
+ * Special handling for ARM IO range
+ * TODO: need to move pci_register_io_range() function out
+ * of drivers/of/address.c for both used by DT and ACPI
+ */
+ if (res->flags & IORESOURCE_IO) {
+ unsigned long port;
+ int err;
+ resource_size_t length = res->end - res->start;
+
+ err = pci_register_io_range(res->start, length);
+ if (err) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ port = pci_address_to_pio(res->start);
+ if (port == (unsigned long)-1) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ res->start = port;
+ res->end = res->start + length - 1;
+
+ if (pci_remap_iospace(res, res->start) < 0)
+ resource_list_destroy_entry(entry);
+ }
+ }
+
+ return 0;
+}
+
+static struct acpi_pci_root_ops acpi_pci_root_ops = {
+ .pci_ops = &pci_root_ops,
+ .init_info = pci_acpi_root_init_info,
+ .release_info = pci_acpi_root_release_info,
+ .prepare_resources = pci_acpi_root_prepare_resources,
+};
+
/* 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;
+ int node = acpi_get_node(root->device->handle);
+ int domain = root->segment;
+ int busnum = root->secondary.start;
+ struct acpi_pci_root_info *info;
+ struct pci_bus *bus;
+
+ if (domain && !pci_domains_supported) {
+ pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
+ if (!info) {
+ dev_err(&root->device->dev,
+ "pci_bus %04x:%02x: ignored (out of memory)\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
+
+ /* After the PCI-E bus has been walked and all devices discovered,
+ * configure any settings of the fabric that might be necessary.
+ */
+ if (bus) {
+ struct pci_bus *child;
+
+ list_for_each_entry(child, &bus->children, node)
+ pcie_bus_configure_settings(child);
+ }
+
+ return bus;
}
#endif

Tomasz,
I was asked to test your new patchset on our platform. With the new
patchset, I'm seeing two problems.

1. ACPI code is unable to discover the interrupt numbers when objects
are ordered as follows in the ACPI file

PNP0A08 object
PNP0C0F INTA object
PNP0C0F INTB object
PNP0C0F INTC object
PNP0C0F INTD object

This gives me invalid link context error.

pci 0000:00:00.0: PCI INT A: no GSI
pci 0000:01:00.0: Derived GSI for 0000:01:00.0 INT A from 0000:00:00.0
acpi PNP0C0F:00: Invalid link context

If I order it like this in the ACPI file,

PNP0C0F INTA object
PNP0C0F INTB object
PNP0C0F INTC object
PNP0C0F INTD object
PNP0A08 object

then, the legacy interrupt numbers can be discovered properly.

2. The second problem is about the PCIe resources.

pci_0000:01:00.0:_can't_enable_device:_BAR_0_[mem_0x80100100000-0x80100101fff_64bit]_not_claimed
pci 0000:01:00.0: Can't enable PCI device, BIOS handoff failed.
pci 0000:00:00.0: BAR 14: assigned [mem 0x80100100000-0x801003fffff]
pci 0000:00:00.0: BAR 13: no space for [io size 0x1000]
pci 0000:00:00.0: BAR 13: failed to assign [io size 0x1000]
pci 0000:00:00.0: BAR 13: no space for [io size 0x1000]
pci 0000:00:00.0: BAR 13: failed to assign [io size 0x1000]
pci 0000:01:00.0: BAR 0: assigned [mem 0x80100100000-0x80100101fff 64bit]

For some reason, the kernel is unable to assign resources.

I appreciate any pointers you might give.

[Tomasz Nowicki]

Post by Sinan Kaya
1. ACPI code is unable to discover the interrupt numbers when objects
are ordered as follows in the ACPI file
PNP0A08 object
PNP0C0F INTA object
PNP0C0F INTB object
PNP0C0F INTC object
PNP0C0F INTD object
This gives me invalid link context error.
pci 0000:00:00.0: PCI INT A: no GSI
pci 0000:01:00.0: Derived GSI for 0000:01:00.0 INT A from 0000:00:00.0
acpi PNP0C0F:00: Invalid link context
If I order it like this in the ACPI file,
PNP0C0F INTA object
PNP0C0F INTB object
PNP0C0F INTC object
PNP0C0F INTD object
PNP0A08 object
then, the legacy interrupt numbers can be discovered properly.

Can you show full content of your PNP0C0F and PNP0A08 objects?

Regards,
Tomasz

[Sinan Kaya]

Post by Tomasz Nowicki

Post by Sinan Kaya
1. ACPI code is unable to discover the interrupt numbers when objects
are ordered as follows in the ACPI file
PNP0A08 object
PNP0C0F INTA object
PNP0C0F INTB object
PNP0C0F INTC object
PNP0C0F INTD object
This gives me invalid link context error.
pci 0000:00:00.0: PCI INT A: no GSI
pci 0000:01:00.0: Derived GSI for 0000:01:00.0 INT A from 0000:00:00.0
acpi PNP0C0F:00: Invalid link context
If I order it like this in the ACPI file,
PNP0C0F INTA object
PNP0C0F INTB object
PNP0C0F INTC object
PNP0C0F INTD object
PNP0A08 object
then, the legacy interrupt numbers can be discovered properly.

Can you show full content of your PNP0C0F and PNP0A08 objects?

ACPI table is considered proprietary. I don't think I can get the legal
approval in time. I can give you pieces though.

Here is the _PRT
Device (PCI0) { // PCIe port 0
Name(_HID, EISAID("PNP0A08")) // PCI express
Name(_CID, EISAID("PNP0A03")) // Compatible PCI Root Bridge
{
....
Name(_PRT, Package(){
Package(){0x0FFFF, 0, \_SB.LN0A, 0}, // Slot 0, INTA
Package(){0x0FFFF, 1, \_SB.LN0B, 0}, // Slot 0, INTB
Package(){0x0FFFF, 2, \_SB.LN0C, 0}, // Slot 0, INTC
Package(){0x0FFFF, 3, \_SB.LN0D, 0} // Slot 0, INTD
})
}

Here is the PNP0C0F

Device(LN0A){
Name(_HID, EISAID("PNP0C0F")) // PCI interrupt link
Name(_UID, 1)
Name(_PRS, ResourceTemplate(){
Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive, , ,) {0xE8}
})
Method(_DIS) {}
Method(_CRS) { Return (_PRS) }
Method(_SRS, 1) {}
}

Post by Tomasz Nowicki
Regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Tomasz Nowicki]

Post by Sinan Kaya

Post by Tomasz Nowicki

Post by Sinan Kaya
1. ACPI code is unable to discover the interrupt numbers when objects
are ordered as follows in the ACPI file
PNP0A08 object
PNP0C0F INTA object
PNP0C0F INTB object
PNP0C0F INTC object
PNP0C0F INTD object
This gives me invalid link context error.
pci 0000:00:00.0: PCI INT A: no GSI
pci 0000:01:00.0: Derived GSI for 0000:01:00.0 INT A from 0000:00:00.0
acpi PNP0C0F:00: Invalid link context
If I order it like this in the ACPI file,
PNP0C0F INTA object
PNP0C0F INTB object
PNP0C0F INTC object
PNP0C0F INTD object
PNP0A08 object
then, the legacy interrupt numbers can be discovered properly.

Can you show full content of your PNP0C0F and PNP0A08 objects?

ACPI table is considered proprietary. I don't think I can get the legal
approval in time. I can give you pieces though.
Here is the _PRT
Device (PCI0) { // PCIe port 0
Name(_HID, EISAID("PNP0A08")) // PCI express
Name(_CID, EISAID("PNP0A03")) // Compatible PCI Root Bridge
{
....
Name(_PRT, Package(){
Package(){0x0FFFF, 0, \_SB.LN0A, 0}, // Slot 0, INTA
Package(){0x0FFFF, 1, \_SB.LN0B, 0}, // Slot 0, INTB
Package(){0x0FFFF, 2, \_SB.LN0C, 0}, // Slot 0, INTC
Package(){0x0FFFF, 3, \_SB.LN0D, 0} // Slot 0, INTD
})
}
Here is the PNP0C0F
Device(LN0A){
Name(_HID, EISAID("PNP0C0F")) // PCI interrupt link
Name(_UID, 1)
Name(_PRS, ResourceTemplate(){
Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive, , ,) {0xE8}
})
Method(_DIS) {}
Method(_CRS) { Return (_PRS) }
Method(_SRS, 1) {}
}

Can you please apply patch below:

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index fec1c91..fe34415 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -48,10 +48,19 @@ resource_size_t pcibios_align_resource(void *data,
const struct resource *res,
*/
int pcibios_enable_device(struct pci_dev *dev, int mask)
{
+ int ret;
+
if (pci_has_flag(PCI_PROBE_ONLY))
return 0;

- return pci_enable_resources(dev, mask);
+ ret = pci_enable_resources(dev, mask);
+ if (ret < 0)
+ return ret;
+#ifdef CONFIG_ACPI
+ if (!dev->msi_enabled)
+ return acpi_pci_irq_enable(dev);
+#endif
+ return 0;
}

/*
@@ -61,10 +70,6 @@ int pcibios_add_device(struct pci_dev *dev)
{
if (acpi_disabled)
dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
-#ifdef CONFIG_ACPI
- else
- acpi_pci_irq_enable(dev);
-#endif

return 0;
}

and let me know if the order still matter?

Regards,
Tomasz

[Sinan Kaya]

Post by Tomasz Nowicki

Post by Sinan Kaya

Post by Tomasz Nowicki

Post by Sinan Kaya
1. ACPI code is unable to discover the interrupt numbers when objects
are ordered as follows in the ACPI file
PNP0A08 object
PNP0C0F INTA object
PNP0C0F INTB object
PNP0C0F INTC object
PNP0C0F INTD object
This gives me invalid link context error.
pci 0000:00:00.0: PCI INT A: no GSI
pci 0000:01:00.0: Derived GSI for 0000:01:00.0 INT A from 0000:00:00.0
acpi PNP0C0F:00: Invalid link context
If I order it like this in the ACPI file,
PNP0C0F INTA object
PNP0C0F INTB object
PNP0C0F INTC object
PNP0C0F INTD object
PNP0A08 object
then, the legacy interrupt numbers can be discovered properly.

Can you show full content of your PNP0C0F and PNP0A08 objects?

ACPI table is considered proprietary. I don't think I can get the legal
approval in time. I can give you pieces though.
Here is the _PRT
Device (PCI0) { // PCIe port 0
Name(_HID, EISAID("PNP0A08")) // PCI express
Name(_CID, EISAID("PNP0A03")) // Compatible PCI Root Bridge
{
....
Name(_PRT, Package(){
Package(){0x0FFFF, 0, \_SB.LN0A, 0}, // Slot 0, INTA
Package(){0x0FFFF, 1, \_SB.LN0B, 0}, // Slot 0, INTB
Package(){0x0FFFF, 2, \_SB.LN0C, 0}, // Slot 0, INTC
Package(){0x0FFFF, 3, \_SB.LN0D, 0} // Slot 0, INTD
})
}
Here is the PNP0C0F
Device(LN0A){
Name(_HID, EISAID("PNP0C0F")) // PCI interrupt link
Name(_UID, 1)
Name(_PRS, ResourceTemplate(){
Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive, , ,) {0xE8}
})
Method(_DIS) {}
Method(_CRS) { Return (_PRS) }
Method(_SRS, 1) {}
}

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index fec1c91..fe34415 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -48,10 +48,19 @@ resource_size_t pcibios_align_resource(void *data,
const struct resource *res,
*/
int pcibios_enable_device(struct pci_dev *dev, int mask)
{
+ int ret;
+
if (pci_has_flag(PCI_PROBE_ONLY))
return 0;
- return pci_enable_resources(dev, mask);
+ ret = pci_enable_resources(dev, mask);
+ if (ret < 0)
+ return ret;
+#ifdef CONFIG_ACPI
+ if (!dev->msi_enabled)
+ return acpi_pci_irq_enable(dev);
+#endif
+ return 0;
}
/*
@@ -61,10 +70,6 @@ int pcibios_add_device(struct pci_dev *dev)
{
if (acpi_disabled)
dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
-#ifdef CONFIG_ACPI
- else
- acpi_pci_irq_enable(dev);
-#endif
return 0;
}
and let me know if the order still matter?
Regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

Thanks,
This seems to have fixed the ACPI table order problem.

[Sinan Kaya]

Post by Sinan Kaya
2. The second problem is about the PCIe resources.
pci_0000:01:00.0:_can't_enable_device:_BAR_0_[mem_0x80100100000-0x80100101fff_64bit]_not_claimed
pci 0000:01:00.0: Can't enable PCI device, BIOS handoff failed.
pci 0000:00:00.0: BAR 14: assigned [mem 0x80100100000-0x801003fffff]
pci 0000:00:00.0: BAR 13: no space for [io size 0x1000]
pci 0000:00:00.0: BAR 13: failed to assign [io size 0x1000]
pci 0000:00:00.0: BAR 13: no space for [io size 0x1000]
pci 0000:00:00.0: BAR 13: failed to assign [io size 0x1000]
pci 0000:01:00.0: BAR 0: assigned [mem 0x80100100000-0x80100101fff 64bit]
For some reason, the kernel is unable to assign resources.
I appreciate any pointers you might give.

Tomasz,
I debugged this part of the problem yesterday night.

I have one set up with 4.2 kernel and Mark Salter's original ACPI PCIE
patch. His patchwork works fine with our ACPI table.

This is what IO resource looks like in our table. PCI IO is supported
using the ACPI translation attribute on ARM systems.

QWORDIO( // produced resource
ResourceProducer, // bit 0 of general flags is 0
MinFixed, // Range is fixed
MaxFixed, // Range is fixed
PosDecode,
EntireRange,
0x0000, // Granularity
0x1000, // Min,
0xFFFF, // Max
0x8FFFFFEF000, // Translation
0xF000, // Range Length
,, PI00
)

It looks like you are missing the translation support for the IO
aperture and Mark Salter's patch has this support.

Yours is missing in pci_acpi_root_prepare_resources function here.
Mark's patch uses the offset argument to reorganize the resource.


+ resource_size_t length = res->end - res->start;
+
+ err = pci_register_io_range(res->start, length);
+ if (err) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ port = pci_address_to_pio(res->start);
+ if (port == (unsigned long)-1) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ res->start = port;
+ res->end = res->start + length - 1;
+
+ if (pci_remap_iospace(res, res->start) < 0)
+ resource_list_destroy_entry(entry);
pci_0000:01:00.0:_can't_enable_device:_BAR_0_[mem_0x80100100000-0x80100101fff_64bit]_not_claimed

Post by Sinan Kaya
pci 0000:01:00.0: Can't enable PCI device, BIOS handoff failed.

I also found out this problem. This is a resource reclaim order problem
with respect to quirks. The card I'm testing is a PCIe USB card. It has
a quirk associated with it and it tries to enable and then disable the
device here.

DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
PCI_CLASS_SERIAL_USB, 8, quirk_usb_early_handoff);

The resources get assigned after this quirk. That's why I see these
can't enable messages. I looked at 4.2 kernel. The execution order is
reversed.

Post by Sinan Kaya
pci 0000:00:00.0: BAR 14: assigned [mem 0x80100100000-0x801003fffff]
pci 0000:00:00.0: BAR 13: no space for [io size 0x1000]
pci 0000:00:00.0: BAR 13: failed to assign [io size 0x1000]
pci 0000:00:00.0: BAR 13: no space for [io size 0x1000]
pci 0000:00:00.0: BAR 13: failed to assign [io size 0x1000]
pci 0000:01:00.0: BAR 0: assigned [mem 0x80100100000-0x80100101fff 64bit]

[Tomasz Nowicki]

Post by Sinan Kaya

Post by Sinan Kaya
2. The second problem is about the PCIe resources.
pci_0000:01:00.0:_can't_enable_device:_BAR_0_[mem_0x80100100000-0x80100101fff_64bit]_not_claimed
pci 0000:01:00.0: Can't enable PCI device, BIOS handoff failed.
pci 0000:00:00.0: BAR 14: assigned [mem 0x80100100000-0x801003fffff]
pci 0000:00:00.0: BAR 13: no space for [io size 0x1000]
pci 0000:00:00.0: BAR 13: failed to assign [io size 0x1000]
pci 0000:00:00.0: BAR 13: no space for [io size 0x1000]
pci 0000:00:00.0: BAR 13: failed to assign [io size 0x1000]
pci 0000:01:00.0: BAR 0: assigned [mem 0x80100100000-0x80100101fff 64bit]
For some reason, the kernel is unable to assign resources.
I appreciate any pointers you might give.

Tomasz,
I debugged this part of the problem yesterday night.
I have one set up with 4.2 kernel and Mark Salter's original ACPI PCIE
patch. His patchwork works fine with our ACPI table.
This is what IO resource looks like in our table. PCI IO is supported
using the ACPI translation attribute on ARM systems.
QWORDIO( // produced resource
ResourceProducer, // bit 0 of general flags is 0
MinFixed, // Range is fixed
MaxFixed, // Range is fixed
PosDecode,
EntireRange,
0x0000, // Granularity
0x1000, // Min,
0xFFFF, // Max
0x8FFFFFEF000, // Translation
0xF000, // Range Length
,, PI00
)
It looks like you are missing the translation support for the IO
aperture and Mark Salter's patch has this support.
Yours is missing in pci_acpi_root_prepare_resources function here.
Mark's patch uses the offset argument to reorganize the resource.
+ resource_size_t length = res->end - res->start;
+
+ err = pci_register_io_range(res->start, length);
+ if (err) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ port = pci_address_to_pio(res->start);
+ if (port == (unsigned long)-1) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ res->start = port;
+ res->end = res->start + length - 1;
+
+ if (pci_remap_iospace(res, res->start) < 0)
+ resource_list_destroy_entry(entry);

Thanks for the heads up, working on it.

Regards,
Tomasz

[Lorenzo Pieralisi]

On Wed, Oct 28, 2015 at 02:46:37PM -0400, Sinan Kaya wrote:

[...]

Post by Sinan Kaya

Post by Tomasz Nowicki
-int raw_pci_write(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 val)
+struct pci_ops pci_root_ops = {
+ .map_bus = pci_mcfg_dev_base,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,

Can you change these with pci_generic_config_read32 and
pci_generic_config_write32? We have some targets that can only do 32
bits PCI config space access.

No.

http://www.spinics.net/lists/linux-pci/msg44869.html

Can you be a bit more specific please ?

Sigh. Looks like we have to start adding platform specific quirks even
before we merged the generic ACPI PCIe host controller implementation.

Lorenzo

Post by Sinan Kaya

Post by Tomasz Nowicki
+};
+
+#ifdef CONFIG_PCI_MMCONFIG
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+ struct acpi_pci_root *root;
+ int seg, start, end, err;
+
+ root = ci->root;
+ seg = root->segment;
+ start = root->secondary.start;
+ end = root->secondary.end;
+
+ cfg = pci_mmconfig_lookup(seg, start);
+ if (cfg)
+ return 0;
+
+ cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
+ if (!cfg)
+ return -ENOMEM;
+
+ err = pci_mmconfig_inject(cfg);
+ return err;
}
-#ifdef CONFIG_ACPI
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ struct acpi_pci_root *root = ci->root;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
+ if (cfg)
+ return;
+
+ if (cfg->hot_added)
+ pci_mmconfig_delete(root->segment, root->secondary.start,
+ root->secondary.end);
+}
+#else
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ return 0;
+}
+
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
+#endif
+
+static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
+{
+ return pci_add_mmconfig_region(ci);
+}
+
+static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
+{
+ pci_remove_mmconfig_region(ci);
+ kfree(ci);
+}
+
+static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
+{
+ struct resource_entry *entry, *tmp;
+ int ret;
+
+ ret = acpi_pci_probe_root_resources(ci);
+ if (ret < 0)
+ return ret;
+
+ resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
+ struct resource *res = entry->res;
+
+ /*
+ * Special handling for ARM IO range
+ * TODO: need to move pci_register_io_range() function out
+ * of drivers/of/address.c for both used by DT and ACPI
+ */
+ if (res->flags & IORESOURCE_IO) {
+ unsigned long port;
+ int err;
+ resource_size_t length = res->end - res->start;
+
+ err = pci_register_io_range(res->start, length);
+ if (err) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ port = pci_address_to_pio(res->start);
+ if (port == (unsigned long)-1) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ res->start = port;
+ res->end = res->start + length - 1;
+
+ if (pci_remap_iospace(res, res->start) < 0)
+ resource_list_destroy_entry(entry);
+ }
+ }
+
+ return 0;
+}
+
+static struct acpi_pci_root_ops acpi_pci_root_ops = {
+ .pci_ops = &pci_root_ops,
+ .init_info = pci_acpi_root_init_info,
+ .release_info = pci_acpi_root_release_info,
+ .prepare_resources = pci_acpi_root_prepare_resources,
+};
+
/* 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;
+ int node = acpi_get_node(root->device->handle);
+ int domain = root->segment;
+ int busnum = root->secondary.start;
+ struct acpi_pci_root_info *info;
+ struct pci_bus *bus;
+
+ if (domain && !pci_domains_supported) {
+ pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
+ if (!info) {
+ dev_err(&root->device->dev,
+ "pci_bus %04x:%02x: ignored (out of memory)\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
+
+ /* After the PCI-E bus has been walked and all devices discovered,
+ * configure any settings of the fabric that might be necessary.
+ */
+ if (bus) {
+ struct pci_bus *child;
+
+ list_for_each_entry(child, &bus->children, node)
+ pcie_bus_configure_settings(child);
+ }
+
+ return bus;
}
#endif

--
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Tomasz Nowicki]

Post by Lorenzo Pieralisi
[...]

Post by Sinan Kaya

Post by Tomasz Nowicki
-int raw_pci_write(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 val)
+struct pci_ops pci_root_ops = {
+ .map_bus = pci_mcfg_dev_base,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,

Can you change these with pci_generic_config_read32 and
pci_generic_config_write32? We have some targets that can only do 32
bits PCI config space access.

No.
http://www.spinics.net/lists/linux-pci/msg44869.html
Can you be a bit more specific please ?
Sigh. Looks like we have to start adding platform specific quirks even
before we merged the generic ACPI PCIe host controller implementation.

The sad reality... But my next version will be still generic. Once that
one appear to be in good shape then we can add quirks.

Regards,
Tomasz

[Sinan Kaya]

Post by Tomasz Nowicki

Post by Lorenzo Pieralisi

Post by Sinan Kaya

Post by Tomasz Nowicki
+struct pci_ops pci_root_ops = {
+ .map_bus = pci_mcfg_dev_base,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,

Can you change these with pci_generic_config_read32 and
pci_generic_config_write32? We have some targets that can only do 32
bits PCI config space access.

No.
http://www.spinics.net/lists/linux-pci/msg44869.html
Can you be a bit more specific please ?
Sigh. Looks like we have to start adding platform specific quirks even
before we merged the generic ACPI PCIe host controller implementation.

The sad reality... But my next version will be still generic. Once that
one appear to be in good shape then we can add quirks.

Thanks.

I don't see anywhere in the SBSA spec addendum that the PCI
configuration space section that unaligned accesses *MUST* be supported.

If this is required, please have this info added to the spec. I can work
with the designers for the next chip.

Unaligned access on the current hardware returns incomplete values or
can cause bus faults. The behavior is undefined.

[Arnd Bergmann]

Post by Sinan Kaya
I don't see anywhere in the SBSA spec addendum that the PCI
configuration space section that unaligned accesses *MUST* be supported.
If this is required, please have this info added to the spec. I can work
with the designers for the next chip.
Unaligned access on the current hardware returns incomplete values or
can cause bus faults. The behavior is undefined.

Unaligned accesses are not allowed, but any PCI compliant device must
support aligned 1, 2 or 4 byte accesses on its configuration space,
though the byte-enable mechanism. In an ECAM host bridge, those are
mapped to load/store accesses from the CPU with the respective width
and natural alignment.

Arnd

[Sinan Kaya]

Post by Arnd Bergmann

Post by Sinan Kaya
I don't see anywhere in the SBSA spec addendum that the PCI
configuration space section that unaligned accesses *MUST* be supported.
If this is required, please have this info added to the spec. I can work
with the designers for the next chip.
Unaligned access on the current hardware returns incomplete values or
can cause bus faults. The behavior is undefined.

Unaligned accesses are not allowed, but any PCI compliant device must
support aligned 1, 2 or 4 byte accesses on its configuration space,
though the byte-enable mechanism. In an ECAM host bridge, those are
mapped to load/store accesses from the CPU with the respective width
and natural alignment.
Arnd

As far as I see, the endpoints do not have any problems with unaligned
accesses. It is the host bridge itself (stuff that doesn't get on the
PCIe bus and uses traditional AXI kind bus internally) has problems with
alignment.

If Linux is expecting all HW vendors to implement alignment support,
this needs to be put in the SBSA spec as a hard requirement.

[Arnd Bergmann]

Post by Sinan Kaya

Post by Arnd Bergmann

Post by Sinan Kaya
I don't see anywhere in the SBSA spec addendum that the PCI
configuration space section that unaligned accesses *MUST* be supported.
If this is required, please have this info added to the spec. I can work
with the designers for the next chip.
Unaligned access on the current hardware returns incomplete values or
can cause bus faults. The behavior is undefined.

Unaligned accesses are not allowed, but any PCI compliant device must
support aligned 1, 2 or 4 byte accesses on its configuration space,
though the byte-enable mechanism. In an ECAM host bridge, those are
mapped to load/store accesses from the CPU with the respective width
and natural alignment.

As far as I see, the endpoints do not have any problems with unaligned
accesses. It is the host bridge itself (stuff that doesn't get on the
PCIe bus and uses traditional AXI kind bus internally) has problems with
alignment.
If Linux is expecting all HW vendors to implement alignment support,
this needs to be put in the SBSA spec as a hard requirement.

As I said, it's not unaligned accesses at all, just 1-byte and aligned
2-byte accesses, and it's not Linux mandating this but the PCI
spec. Please read Russell's email again, it is not possible for PCI
to work according to the specification unless the host bridge allows
sub-32-bit accesses.

You can probably work around this by using the legacy I/O port method
rather than ECAM, if the PCI host bridge itself is functional and just
the host bus it is connected to is buggy.

Arnd

[Sinan Kaya]

Post by Arnd Bergmann

Post by Sinan Kaya

Post by Arnd Bergmann

Post by Sinan Kaya
I don't see anywhere in the SBSA spec addendum that the PCI
configuration space section that unaligned accesses *MUST* be supported.
If this is required, please have this info added to the spec. I can work
with the designers for the next chip.
Unaligned access on the current hardware returns incomplete values or
can cause bus faults. The behavior is undefined.

Unaligned accesses are not allowed, but any PCI compliant device must
support aligned 1, 2 or 4 byte accesses on its configuration space,
though the byte-enable mechanism. In an ECAM host bridge, those are
mapped to load/store accesses from the CPU with the respective width
and natural alignment.

As far as I see, the endpoints do not have any problems with unaligned
accesses. It is the host bridge itself (stuff that doesn't get on the
PCIe bus and uses traditional AXI kind bus internally) has problems with
alignment.
If Linux is expecting all HW vendors to implement alignment support,
this needs to be put in the SBSA spec as a hard requirement.

As I said, it's not unaligned accesses at all, just 1-byte and aligned
2-byte accesses, and it's not Linux mandating this but the PCI
spec. Please read Russell's email again, it is not possible for PCI
to work according to the specification unless the host bridge allows
sub-32-bit accesses.

I'll check back with the hardware designers. Seeing readb/readw/readl
made me nervous that we are trying unaligned access from any boundaries.

In any case, the hardware document says 32 bit configuration space
access to the host bridge only. I'll get more clarification.

Post by Arnd Bergmann
You can probably work around this by using the legacy I/O port method
rather than ECAM, if the PCI host bridge itself is functional and just
the host bus it is connected to is buggy.

From the sounds of it, we'll need a quirk for config space. We support
legacy I/O only to make the endpoints happy. Some endpoints do not get
initialized if they don't have a BAR address assigned to all the BAR
resources.

I just saw David Daney's email. I like his idea. I think this chip will
fit into the same category.

Post by Arnd Bergmann
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Sinan Kaya]

Post by Sinan Kaya
In any case, the hardware document says 32 bit configuration space
access to the host bridge only. I'll get more clarification.

I got confirmation this morning that this chip supports 32 bit access to
the root complex configuration space. 8/16/32 bits accesses to the
endpoints are supported.

Post by Sinan Kaya

Post by Arnd Bergmann
You can probably work around this by using the legacy I/O port method
rather than ECAM, if the PCI host bridge itself is functional and just
the host bus it is connected to is buggy.

From the sounds of it, we'll need a quirk for config space. We support
legacy I/O only to make the endpoints happy. Some endpoints do not get
initialized if they don't have a BAR address assigned to all the BAR
resources.

We'll need an MCFG fix up.

[Hanjun Guo]

Post by Lorenzo Pieralisi
[...]

Post by Sinan Kaya

Post by Tomasz Nowicki
-int raw_pci_write(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 val)
+struct pci_ops pci_root_ops = {
+ .map_bus = pci_mcfg_dev_base,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,

Can you change these with pci_generic_config_read32 and
pci_generic_config_write32? We have some targets that can only do 32
bits PCI config space access.

No.
http://www.spinics.net/lists/linux-pci/msg44869.html
Can you be a bit more specific please ?
Sigh. Looks like we have to start adding platform specific quirks even
before we merged the generic ACPI PCIe host controller implementation.

Cc Gab, Zhou, and Dondong who upstream the hip05 (designware) PCIe host
support.

I think so, some platform may not support ECAM for root complex,
which needs special handling of access config space, we may need
to consider those cases.

Thanks
Hanjun

[David Daney]

Post by Hanjun Guo

Post by Lorenzo Pieralisi
[...]

Post by Sinan Kaya

Post by Tomasz Nowicki
-int raw_pci_write(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 val)
+struct pci_ops pci_root_ops = {
+ .map_bus = pci_mcfg_dev_base,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,

Can you change these with pci_generic_config_read32 and
pci_generic_config_write32? We have some targets that can only do 32
bits PCI config space access.

No.
http://www.spinics.net/lists/linux-pci/msg44869.html
Can you be a bit more specific please ?
Sigh. Looks like we have to start adding platform specific quirks even
before we merged the generic ACPI PCIe host controller implementation.

Cc Gab, Zhou, and Dondong who upstream the hip05 (designware) PCIe host
support.
I think so, some platform may not support ECAM for root complex,
which needs special handling of access config space, we may need
to consider those cases.

Yes, it is indeed true. For example, some Cavium ThunderX processors
fall into this category.

Some options I thought of are:

o Use DECLARE_ACPI_MCFG_FIXUP() in the kernel to supply the needed
config space accessors.

o Define additional root_device_ids that imply the needed config space
accessors.

Post by Hanjun Guo
Thanks
Hanjun

[Gabriele Paoloni]

Hi David

-----Original Message-----
Sent: 03 November 2015 17:39
To: Hanjun Guo
(B); liudongdong (C)
Subject: Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI
hostbridge init

Post by Hanjun Guo

Post by Lorenzo Pieralisi
[...]

Post by Sinan Kaya

Post by Tomasz Nowicki
-int raw_pci_write(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 val)
+struct pci_ops pci_root_ops = {
+ .map_bus = pci_mcfg_dev_base,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,

Can you change these with pci_generic_config_read32 and
pci_generic_config_write32? We have some targets that can only do 32
bits PCI config space access.

No.
http://www.spinics.net/lists/linux-pci/msg44869.html
Can you be a bit more specific please ?
Sigh. Looks like we have to start adding platform specific quirks even
before we merged the generic ACPI PCIe host controller implementation.

Cc Gab, Zhou, and Dondong who upstream the hip05 (designware) PCIe host
support.
I think so, some platform may not support ECAM for root complex,
which needs special handling of access config space, we may need
to consider those cases.

Yes, it is indeed true. For example, some Cavium ThunderX processors
fall into this category.
o Use DECLARE_ACPI_MCFG_FIXUP() in the kernel to supply the needed
config space accessors.
o Define additional root_device_ids that imply the needed config space
accessors.

Yes I like this

it would fit designware too

Gab

Post by Hanjun Guo
Thanks
Hanjun

[Lorenzo Pieralisi]

On Tue, Oct 27, 2015 at 05:38:42PM +0100, Tomasz Nowicki wrote:

[...]

Post by Tomasz Nowicki
menu "Kernel Features"
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index b3d098b..66cc1ae 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -11,12 +11,15 @@
*/
#include <linux/acpi.h>
+#include <linux/ecam.h>
#include <linux/init.h>
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/mm.h>
+#include <linux/of_address.h>
#include <linux/of_pci.h>
#include <linux/of_platform.h>
+#include <linux/pci-acpi.h>
#include <linux/slab.h>
#include <asm/pci-bridge.h>
@@ -52,35 +55,216 @@ 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 from DT/ACPI when adding a new device
*/
int pcibios_add_device(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
+ acpi_pci_irq_enable(dev);
+#endif

This series:

http://www.spinics.net/lists/linux-pci/msg45950.html

will allow us to initialize the irq mapping function according to
the boot method, code above is getting cumbersome and it is already
overriden when booting with DT, so we will remove it altogether.

Post by Tomasz Nowicki
return 0;
}
+#ifdef CONFIG_ACPI
+int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
+{
+ struct acpi_pci_root *root = bridge->bus->sysdata;
+
+ ACPI_COMPANION_SET(&bridge->dev, root->device);
+ return 0;

This should be made part of core code IMO.

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

Two functions above are identical for arm64, ia64 and x86, I do
not think they belong in arch code.

Post by Tomasz Nowicki
+static int __init pcibios_assign_resources(void)
+{
+ if (acpi_disabled)
+ return 0;
+
+ pci_assign_unassigned_resources();
+ return 0;

Already commented on this.

Post by Tomasz Nowicki
+}
/*
- * raw_pci_read/write - Platform-specific PCI config space access.
+ * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
+ * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
*/
-int raw_pci_read(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 *val)
+rootfs_initcall(pcibios_assign_resources);
+
+static void __iomem *
+pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
+ if (cfg && cfg->virt)
+ return cfg->virt +
+ (PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
+ offset;
+ return NULL;

Why is this code arm64 specific ?

Post by Tomasz Nowicki
}
-int raw_pci_write(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 val)
+struct pci_ops pci_root_ops = {
+ .map_bus = pci_mcfg_dev_base,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,
+};
+
+#ifdef CONFIG_PCI_MMCONFIG
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+ struct acpi_pci_root *root;
+ int seg, start, end, err;
+
+ root = ci->root;
+ seg = root->segment;
+ start = root->secondary.start;
+ end = root->secondary.end;
+
+ cfg = pci_mmconfig_lookup(seg, start);
+ if (cfg)
+ return 0;
+
+ cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
+ if (!cfg)
+ return -ENOMEM;
+
+ err = pci_mmconfig_inject(cfg);
+ return err;
}
-#ifdef CONFIG_ACPI
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ struct acpi_pci_root *root = ci->root;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
+ if (cfg)
+ return;
+
+ if (cfg->hot_added)
+ pci_mmconfig_delete(root->segment, root->secondary.start,
+ root->secondary.end);
+}
+#else
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ return 0;
+}
+
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
+#endif

Ditto.

Post by Tomasz Nowicki
+
+static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
+{
+ return pci_add_mmconfig_region(ci);
+}
+
+static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
+{
+ pci_remove_mmconfig_region(ci);
+ kfree(ci);
+}
+
+static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
+{
+ struct resource_entry *entry, *tmp;
+ int ret;
+
+ ret = acpi_pci_probe_root_resources(ci);
+ if (ret < 0)
+ return ret;

Code above is identical on arm64, ia64 and x86.

Post by Tomasz Nowicki
+
+ resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
+ struct resource *res = entry->res;
+
+ /*
+ * Special handling for ARM IO range
+ * TODO: need to move pci_register_io_range() function out
+ * of drivers/of/address.c for both used by DT and ACPI
+ */
+ if (res->flags & IORESOURCE_IO) {
+ unsigned long port;
+ int err;
+ resource_size_t length = res->end - res->start;
+
+ err = pci_register_io_range(res->start, length);
+ if (err) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ port = pci_address_to_pio(res->start);
+ if (port == (unsigned long)-1) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ res->start = port;
+ res->end = res->start + length - 1;
+
+ if (pci_remap_iospace(res, res->start) < 0)
+ resource_list_destroy_entry(entry);
+ }
+ }
+
+ return 0;
+}
+
+static struct acpi_pci_root_ops acpi_pci_root_ops = {
+ .pci_ops = &pci_root_ops,
+ .init_info = pci_acpi_root_init_info,
+ .release_info = pci_acpi_root_release_info,
+ .prepare_resources = pci_acpi_root_prepare_resources,
+};
+
/* 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;
+ int node = acpi_get_node(root->device->handle);
+ int domain = root->segment;
+ int busnum = root->secondary.start;
+ struct acpi_pci_root_info *info;
+ struct pci_bus *bus;
+
+ if (domain && !pci_domains_supported) {
+ pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
+ if (!info) {
+ dev_err(&root->device->dev,
+ "pci_bus %04x:%02x: ignored (out of memory)\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
+
+ /* After the PCI-E bus has been walked and all devices discovered,
+ * configure any settings of the fabric that might be necessary.
+ */
+ if (bus) {
+ struct pci_bus *child;
+
+ list_for_each_entry(child, &bus->children, node)
+ pcie_bus_configure_settings(child);
+ }
+
+ return bus;

Code above is entirely arch agnostic (apart from what data is passed to
sysdata) and I think there is room for further consolidation with
x86 and ia64, I will have a look into this.

Thanks,
Lorenzo

[Tomasz Nowicki]

Post by Lorenzo Pieralisi
[...]

Post by Tomasz Nowicki
menu "Kernel Features"
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index b3d098b..66cc1ae 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -11,12 +11,15 @@
*/
#include <linux/acpi.h>
+#include <linux/ecam.h>
#include <linux/init.h>
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/mm.h>
+#include <linux/of_address.h>
#include <linux/of_pci.h>
#include <linux/of_platform.h>
+#include <linux/pci-acpi.h>
#include <linux/slab.h>
#include <asm/pci-bridge.h>
@@ -52,35 +55,216 @@ 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 from DT/ACPI when adding a new device
*/
int pcibios_add_device(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
+ acpi_pci_irq_enable(dev);
+#endif

http://www.spinics.net/lists/linux-pci/msg45950.html
will allow us to initialize the irq mapping function according to
the boot method, code above is getting cumbersome and it is already
overriden when booting with DT, so we will remove it altogether.

Post by Tomasz Nowicki
return 0;
}
+#ifdef CONFIG_ACPI
+int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
+{
+ struct acpi_pci_root *root = bridge->bus->sysdata;
+
+ ACPI_COMPANION_SET(&bridge->dev, root->device);
+ return 0;

This should be made part of core code IMO.

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

Two functions above are identical for arm64, ia64 and x86, I do
not think they belong in arch code.

Post by Tomasz Nowicki
+static int __init pcibios_assign_resources(void)
+{
+ if (acpi_disabled)
+ return 0;
+
+ pci_assign_unassigned_resources();
+ return 0;

Already commented on this.

Post by Tomasz Nowicki
+}
/*
- * raw_pci_read/write - Platform-specific PCI config space access.
+ * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
+ * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
*/
-int raw_pci_read(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 *val)
+rootfs_initcall(pcibios_assign_resources);
+
+static void __iomem *
+pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
+ if (cfg && cfg->virt)
+ return cfg->virt +
+ (PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
+ offset;
+ return NULL;

Why is this code arm64 specific ?

Post by Tomasz Nowicki
}
-int raw_pci_write(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 val)
+struct pci_ops pci_root_ops = {
+ .map_bus = pci_mcfg_dev_base,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,
+};
+
+#ifdef CONFIG_PCI_MMCONFIG
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+ struct acpi_pci_root *root;
+ int seg, start, end, err;
+
+ root = ci->root;
+ seg = root->segment;
+ start = root->secondary.start;
+ end = root->secondary.end;
+
+ cfg = pci_mmconfig_lookup(seg, start);
+ if (cfg)
+ return 0;
+
+ cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
+ if (!cfg)
+ return -ENOMEM;
+
+ err = pci_mmconfig_inject(cfg);
+ return err;
}
-#ifdef CONFIG_ACPI
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ struct acpi_pci_root *root = ci->root;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
+ if (cfg)
+ return;
+
+ if (cfg->hot_added)
+ pci_mmconfig_delete(root->segment, root->secondary.start,
+ root->secondary.end);
+}
+#else
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+ return 0;
+}
+
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
+#endif

Ditto.

Post by Tomasz Nowicki
+
+static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
+{
+ return pci_add_mmconfig_region(ci);
+}
+
+static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
+{
+ pci_remove_mmconfig_region(ci);
+ kfree(ci);
+}
+
+static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
+{
+ struct resource_entry *entry, *tmp;
+ int ret;
+
+ ret = acpi_pci_probe_root_resources(ci);
+ if (ret < 0)
+ return ret;

Code above is identical on arm64, ia64 and x86.

Post by Tomasz Nowicki
+
+ resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
+ struct resource *res = entry->res;
+
+ /*
+ * Special handling for ARM IO range
+ * TODO: need to move pci_register_io_range() function out
+ * of drivers/of/address.c for both used by DT and ACPI
+ */
+ if (res->flags & IORESOURCE_IO) {
+ unsigned long port;
+ int err;
+ resource_size_t length = res->end - res->start;
+
+ err = pci_register_io_range(res->start, length);
+ if (err) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ port = pci_address_to_pio(res->start);
+ if (port == (unsigned long)-1) {
+ resource_list_destroy_entry(entry);
+ continue;
+ }
+
+ res->start = port;
+ res->end = res->start + length - 1;
+
+ if (pci_remap_iospace(res, res->start) < 0)
+ resource_list_destroy_entry(entry);
+ }
+ }
+
+ return 0;
+}
+
+static struct acpi_pci_root_ops acpi_pci_root_ops = {
+ .pci_ops = &pci_root_ops,
+ .init_info = pci_acpi_root_init_info,
+ .release_info = pci_acpi_root_release_info,
+ .prepare_resources = pci_acpi_root_prepare_resources,
+};
+
/* 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;
+ int node = acpi_get_node(root->device->handle);
+ int domain = root->segment;
+ int busnum = root->secondary.start;
+ struct acpi_pci_root_info *info;
+ struct pci_bus *bus;
+
+ if (domain && !pci_domains_supported) {
+ pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
+ if (!info) {
+ dev_err(&root->device->dev,
+ "pci_bus %04x:%02x: ignored (out of memory)\n",
+ domain, busnum);
+ return NULL;
+ }
+
+ bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
+
+ /* After the PCI-E bus has been walked and all devices discovered,
+ * configure any settings of the fabric that might be necessary.
+ */
+ if (bus) {
+ struct pci_bus *child;
+
+ list_for_each_entry(child, &bus->children, node)
+ pcie_bus_configure_settings(child);
+ }
+
+ return bus;

Code above is entirely arch agnostic (apart from what data is passed to
sysdata) and I think there is room for further consolidation with
x86 and ia64, I will have a look into this.

I agree with your comments, currently I do not know how much of it can
be consolidated but I will rework my next version in this direction.

Thanks,
Tomasz

[Tomasz Nowicki]

Post by Lorenzo Pieralisi
[...]

Post by Tomasz Nowicki
menu "Kernel Features"
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index b3d098b..66cc1ae 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -11,12 +11,15 @@
*/
#include <linux/acpi.h>
+#include <linux/ecam.h>
#include <linux/init.h>
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/mm.h>
+#include <linux/of_address.h>
#include <linux/of_pci.h>
#include <linux/of_platform.h>
+#include <linux/pci-acpi.h>
#include <linux/slab.h>
#include <asm/pci-bridge.h>
@@ -52,35 +55,216 @@ 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 from DT/ACPI when adding a new device
*/
int pcibios_add_device(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
+ acpi_pci_irq_enable(dev);
+#endif

http://www.spinics.net/lists/linux-pci/msg45950.html
will allow us to initialize the irq mapping function according to
the boot method, code above is getting cumbersome and it is already
overriden when booting with DT, so we will remove it altogether.

Post by Tomasz Nowicki
return 0;
}
+#ifdef CONFIG_ACPI
+int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
+{
+ struct acpi_pci_root *root = bridge->bus->sysdata;
+
+ ACPI_COMPANION_SET(&bridge->dev, root->device);
+ return 0;

This should be made part of core code IMO.

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

Two functions above are identical for arm64, ia64 and x86, I do
not think they belong in arch code.

Post by Tomasz Nowicki
+static int __init pcibios_assign_resources(void)
+{
+ if (acpi_disabled)
+ return 0;
+
+ pci_assign_unassigned_resources();
+ return 0;

Already commented on this.

Post by Tomasz Nowicki
+}
/*
- * raw_pci_read/write - Platform-specific PCI config space access.
+ * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
+ * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
*/
-int raw_pci_read(unsigned int domain, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 *val)
+rootfs_initcall(pcibios_assign_resources);
+
+static void __iomem *
+pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
{
- return -ENXIO;
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
+ if (cfg && cfg->virt)
+ return cfg->virt +
+ (PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
+ offset;
+ return NULL;

Why is this code arm64 specific ?

It is not, I will move it out of here, probably to mcfg.c file where we
can apply quirks.

Thanks,
Tomasz

[Jon Masters]

Hi Tomasz,

Thanks for posting this series.

1. Making MMCONFIG code arch-agnostic which allows all architectures to collect
PCI config regions and used when necessary.
2. Using generic MMCONFIG code and introducing ACPI based PCI hostbridge
initialization for ARM64

Can I confirm that the intention here is that this replaces Hanjun's
previous patch series? Here's the previous one we were tracking:

https://lkml.kernel.org/g/1432644564-24746-1-git-send-email-***@linaro.org

Assuming that is the case, then we will ping a number of folks to
conduct testing and provide acks (this has already been discussed in a
number of conversations with semiconductors over the past few days).

Thanks,

Jon.

[Hanjun Guo]

Hi Jon,

Post by Jon Masters
Hi Tomasz,
Thanks for posting this series.

1. Making MMCONFIG code arch-agnostic which allows all architectures to collect
PCI config regions and used when necessary.
2. Using generic MMCONFIG code and introducing ACPI based PCI hostbridge
initialization for ARM64

Can I confirm that the intention here is that this replaces Hanjun's

Yes, that's the new version according to the discussion from my version.

Post by Jon Masters
Assuming that is the case, then we will ping a number of folks to
conduct testing and provide acks (this has already been discussed in a
number of conversations with semiconductors over the past few days).

Much appreciated.

Thanks
Hanjun

[Tomasz Nowicki]

Post by Jon Masters
Hi Tomasz,
Thanks for posting this series.

1. Making MMCONFIG code arch-agnostic which allows all architectures to collect
PCI config regions and used when necessary.
2. Using generic MMCONFIG code and introducing ACPI based PCI hostbridge
initialization for ARM64

Can I confirm that the intention here is that this replaces Hanjun's

Indeed, we can call it next version.

Post by Jon Masters
Assuming that is the case, then we will ping a number of folks to
conduct testing and provide acks (this has already been discussed in a
number of conversations with semiconductors over the past few days).

Yes, please, thanks!

Tomasz

[Suravee Suthikulpanit]

Tested on AMD Seattle platform (Overdrive revB) w/ both MSI and legacy
interrupt.

Tested-by: Suravee Suthikulpanit <***@amd.com>

Thanks,
Suravee

1. Making MMCONFIG code arch-agnostic which allows all architectures to collect
PCI config regions and used when necessary.
2. Using generic MMCONFIG code and introducing ACPI based PCI hostbridge
initialization for ARM64
[Patch v7 0/7] Consolidate ACPI PCI root common code into ACPI core
https://lkml.org/lkml/2015/10/14/31
https://git.linaro.org/leg/acpi/acpi.git/shortlog/refs/heads/pci-acpi-upstream
This has been tested on Cavium ThunderX 1 socket server.
Any help in reviewing and testing is very appreciated.
XEN / PCI: Remove the dependence on arch x86 when PCI_MMCONFIG=y
x86, pci: Reorder logic of pci_mmconfig_insert() function
x86, pci, acpi: Move arch-agnostic MMCONFIG (aka ECAM) and ACPI code
out of arch/x86/ directory
pci, acpi, mcfg: Provide generic implementation of MCFG code
initialization.
x86, pci: mmconfig_{32,64}.c code refactoring - remove code
duplication.
x86, pci, ecam: mmconfig_64.c becomes default implementation for ECAM
driver.
pci, acpi, mcfg: Provide default RAW ACPI PCI config space accessors.
pci, acpi, ecam: Add flag to indicate whether ECAM region was hot
added or not.
x86, pci: Use previously added ECAM hot_added flag to remove ECAM
regions.
pci, acpi: Provide generic way to assign bus domain number.
arm64, pci, acpi: Support for ACPI based PCI hostbridge init
arch/arm64/Kconfig | 6 +
arch/arm64/kernel/pci.c | 208 ++++++++++++++++++++++++++++++++--
arch/x86/Kconfig | 4 +
arch/x86/include/asm/pci_x86.h | 28 +----
arch/x86/pci/acpi.c | 17 +--
arch/x86/pci/mmconfig-shared.c | 250 +++++++----------------------------------
arch/x86/pci/mmconfig_32.c | 11 +-
arch/x86/pci/mmconfig_64.c | 67 +----------
arch/x86/pci/numachip.c | 1 +
drivers/acpi/Makefile | 1 +
drivers/acpi/mcfg.c | 104 +++++++++++++++++
drivers/acpi/pci_root.c | 2 +-
drivers/pci/Kconfig | 10 ++
drivers/pci/Makefile | 5 +
drivers/pci/ecam.c | 234 ++++++++++++++++++++++++++++++++++++++
drivers/pci/pci.c | 30 ++++-
drivers/xen/pci.c | 7 +-
include/linux/acpi.h | 2 +
include/linux/ecam.h | 44 ++++++++
19 files changed, 691 insertions(+), 340 deletions(-)
create mode 100644 drivers/acpi/mcfg.c
create mode 100644 drivers/pci/ecam.c
create mode 100644 include/linux/ecam.h

[Bjorn Helgaas]

1. Making MMCONFIG code arch-agnostic which allows all architectures to collect
PCI config regions and used when necessary.
2. Using generic MMCONFIG code and introducing ACPI based PCI hostbridge
initialization for ARM64
[Patch v7 0/7] Consolidate ACPI PCI root common code into ACPI core
https://lkml.org/lkml/2015/10/14/31
https://git.linaro.org/leg/acpi/acpi.git/shortlog/refs/heads/pci-acpi-upstream
This has been tested on Cavium ThunderX 1 socket server.
Any help in reviewing and testing is very appreciated.
XEN / PCI: Remove the dependence on arch x86 when PCI_MMCONFIG=y
x86, pci: Reorder logic of pci_mmconfig_insert() function
x86, pci, acpi: Move arch-agnostic MMCONFIG (aka ECAM) and ACPI code
out of arch/x86/ directory
pci, acpi, mcfg: Provide generic implementation of MCFG code
initialization.
x86, pci: mmconfig_{32,64}.c code refactoring - remove code
duplication.
x86, pci, ecam: mmconfig_64.c becomes default implementation for ECAM
driver.
pci, acpi, mcfg: Provide default RAW ACPI PCI config space accessors.
pci, acpi, ecam: Add flag to indicate whether ECAM region was hot
added or not.
x86, pci: Use previously added ECAM hot_added flag to remove ECAM
regions.
pci, acpi: Provide generic way to assign bus domain number.
arm64, pci, acpi: Support for ACPI based PCI hostbridge init
arch/arm64/Kconfig | 6 +
arch/arm64/kernel/pci.c | 208 ++++++++++++++++++++++++++++++++--
arch/x86/Kconfig | 4 +
arch/x86/include/asm/pci_x86.h | 28 +----
arch/x86/pci/acpi.c | 17 +--
arch/x86/pci/mmconfig-shared.c | 250 +++++++----------------------------------
arch/x86/pci/mmconfig_32.c | 11 +-
arch/x86/pci/mmconfig_64.c | 67 +----------
arch/x86/pci/numachip.c | 1 +
drivers/acpi/Makefile | 1 +
drivers/acpi/mcfg.c | 104 +++++++++++++++++
drivers/acpi/pci_root.c | 2 +-
drivers/pci/Kconfig | 10 ++
drivers/pci/Makefile | 5 +
drivers/pci/ecam.c | 234 ++++++++++++++++++++++++++++++++++++++
drivers/pci/pci.c | 30 ++++-
drivers/xen/pci.c | 7 +-
include/linux/acpi.h | 2 +
include/linux/ecam.h | 44 ++++++++
19 files changed, 691 insertions(+), 340 deletions(-)
create mode 100644 drivers/acpi/mcfg.c
create mode 100644 drivers/pci/ecam.c
create mode 100644 include/linux/ecam.h

Waiting for an updated version after all the discussion.

Bjorn

[Jayachandran C.]

[trimmed the cc list a bit]
[...]

Post by Bjorn Helgaas

x86, pci: Reorder logic of pci_mmconfig_insert() function
x86, pci, acpi: Move arch-agnostic MMCONFIG (aka ECAM) and ACPI code
out of arch/x86/ directory
pci, acpi, mcfg: Provide generic implementation of MCFG code
initialization.
x86, pci: mmconfig_{32,64}.c code refactoring - remove code
duplication.
x86, pci, ecam: mmconfig_64.c becomes default implementation for ECAM
driver.
pci, acpi, mcfg: Provide default RAW ACPI PCI config space accessors.
pci, acpi, ecam: Add flag to indicate whether ECAM region was hot
added or not.
x86, pci: Use previously added ECAM hot_added flag to remove ECAM
regions.

[...]

Post by Bjorn Helgaas
Waiting for an updated version after all the discussion.

Hi Bjorn,

After looking thru the code, I don't think moving the mmconfig code out
of x86 and using it in ARM64 is a good idea.

The code maintains a list of mapping of ECAM regions which can modified
at run time, and ends doing this sequence for doing a config access:

rcu_read_lock();
addr = pci_dev_base(seg, bus, devfn);
list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)
...find mapping...
do pci operation
rcu_read_unlock();

A lot of the complexity in mmconfig_*.c is to maintain and validate
the pci_mmcfg_list which seems to be unecessary.

The better way would be to keep the mapping of ECAM region in the PCI
host bridge sysdata like pci-host-generic and directly access using
map_bus/read/write.

I see that early raw_pci_read/raw_pci_write may need to be supported.
I am not sure if this is needed in ARM64 - and if it is, we can handle
this by taking an early mapping for ECAM regions until the PCI host
bridges are setup, and dropping the mapping at that point.

Thanks,
JC.

[Lorenzo Pieralisi]

Post by Jayachandran C.
[trimmed the cc list a bit]
[...]

Post by Bjorn Helgaas

x86, pci: Reorder logic of pci_mmconfig_insert() function
x86, pci, acpi: Move arch-agnostic MMCONFIG (aka ECAM) and ACPI code
out of arch/x86/ directory
pci, acpi, mcfg: Provide generic implementation of MCFG code
initialization.
x86, pci: mmconfig_{32,64}.c code refactoring - remove code
duplication.
x86, pci, ecam: mmconfig_64.c becomes default implementation for ECAM
driver.
pci, acpi, mcfg: Provide default RAW ACPI PCI config space accessors.
pci, acpi, ecam: Add flag to indicate whether ECAM region was hot
added or not.
x86, pci: Use previously added ECAM hot_added flag to remove ECAM
regions.

[...]

Post by Bjorn Helgaas
Waiting for an updated version after all the discussion.

Hi Bjorn,
After looking thru the code, I don't think moving the mmconfig code out
of x86 and using it in ARM64 is a good idea.
The code maintains a list of mapping of ECAM regions which can modified
rcu_read_lock();
addr = pci_dev_base(seg, bus, devfn);
list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)
...find mapping...
do pci operation
rcu_read_unlock();
A lot of the complexity in mmconfig_*.c is to maintain and validate
the pci_mmcfg_list which seems to be unecessary.

That list is there to manage hotplug bridges, what makes you think
it is not necessary ? Jiang (in CC) can certainly comment on that and
how that list handling can be updated/simplified, if possible.

Post by Jayachandran C.
The better way would be to keep the mapping of ECAM region in the PCI
host bridge sysdata like pci-host-generic and directly access using
map_bus/read/write.
I see that early raw_pci_read/raw_pci_write may need to be supported.
I am not sure if this is needed in ARM64 - and if it is, we can handle
this by taking an early mapping for ECAM regions until the PCI host
bridges are setup, and dropping the mapping at that point.

I do not think it is needed (and if it is it can be added later, it
should not block this series), this was already debated in previous
threads (and in the ASWG, where basically nobody could provide a reason
why those raw accessors in ACPICA are meant to exist, they are there for
AML to access config space, why that has to be happen before PCI busses
are enumerated is still a mystery to me).

Lorenzo

[Jayachandran C.]

Post by Lorenzo Pieralisi

Post by Jayachandran C.
[trimmed the cc list a bit]
[...]

Post by Bjorn Helgaas

x86, pci: Reorder logic of pci_mmconfig_insert() function
x86, pci, acpi: Move arch-agnostic MMCONFIG (aka ECAM) and ACPI code
out of arch/x86/ directory
pci, acpi, mcfg: Provide generic implementation of MCFG code
initialization.
x86, pci: mmconfig_{32,64}.c code refactoring - remove code
duplication.
x86, pci, ecam: mmconfig_64.c becomes default implementation for ECAM
driver.
pci, acpi, mcfg: Provide default RAW ACPI PCI config space accessors.
pci, acpi, ecam: Add flag to indicate whether ECAM region was hot
added or not.
x86, pci: Use previously added ECAM hot_added flag to remove ECAM
regions.

[...]

Post by Bjorn Helgaas
Waiting for an updated version after all the discussion.

Hi Bjorn,
After looking thru the code, I don't think moving the mmconfig code out
of x86 and using it in ARM64 is a good idea.
The code maintains a list of mapping of ECAM regions which can modified
rcu_read_lock();
addr = pci_dev_base(seg, bus, devfn);
list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)
...find mapping...
do pci operation
rcu_read_unlock();
A lot of the complexity in mmconfig_*.c is to maintain and validate
the pci_mmcfg_list which seems to be unecessary.

That list is there to manage hotplug bridges, what makes you think
it is not necessary ? Jiang (in CC) can certainly comment on that and
how that list handling can be updated/simplified, if possible.

Looking thru the code, I think moving the MCFG code to common can be
done in a simpler way. I have posted a new patchset for this.
(And I don't see how the hotplug case needs the list, but that is not
relevant now)

Post by Lorenzo Pieralisi

Post by Jayachandran C.
The better way would be to keep the mapping of ECAM region in the PCI
host bridge sysdata like pci-host-generic and directly access using
map_bus/read/write.
I see that early raw_pci_read/raw_pci_write may need to be supported.
I am not sure if this is needed in ARM64 - and if it is, we can handle
this by taking an early mapping for ECAM regions until the PCI host
bridges are setup, and dropping the mapping at that point.

I do not think it is needed (and if it is it can be added later, it
should not block this series), this was already debated in previous
threads (and in the ASWG, where basically nobody could provide a reason
why those raw accessors in ACPICA are meant to exist, they are there for
AML to access config space, why that has to be happen before PCI busses
are enumerated is still a mystery to me).

The posted patchset adds raw_pci_read/raw_pci_write as well.

JC.

[Lorenzo Pieralisi]

On Wed, Dec 16, 2015 at 06:21:37PM +0530, Jayachandran C. wrote:

[...]

Post by Jayachandran C.

Post by Lorenzo Pieralisi
That list is there to manage hotplug bridges, what makes you think
it is not necessary ? Jiang (in CC) can certainly comment on that and
how that list handling can be updated/simplified, if possible.

Looking thru the code, I think moving the MCFG code to common can be
done in a simpler way. I have posted a new patchset for this.
(And I don't see how the hotplug case needs the list, but that is not
relevant now)

You must work with Tomasz and come up with a unified patchset that
implements ACPI PCIe support for ARM64. We do not need more churn, we
need a working set, period. Posting separate RFCs just adds to the
confusion and to maintainers' review backlog, with the end result of
confusing everyone and achieving nothing.

Lorenzo

[Jeremy Linton]

1. Making MMCONFIG code arch-agnostic which allows all architectures to collect
PCI config regions and used when necessary.
2. Using generic MMCONFIG code and introducing ACPI based PCI hostbridge
initialization for ARM64

Tested with legacy interrupts on ARM Juno R1, HP m400 (APM Potenza), and
APM mustang. The latter two platforms have an additional pci quirk patch
applied (to be posted shortly) to fix their problems with the config
space accessors.

Tested-by: Jeremy Linton <***@arm.com>

Thanks!