On Mon, Aug 08, 2016 at 03:05:37PM +0200, Tomasz Nowicki wrote:

pci_config_window keeps pointer to pci_ecam_ops and every time we want to deallocate pci_config_window (pci_ecam_free()) we need to make sure to free pci_ecam_ops in case it was dynamically allocated prior to pci_ecam_create() call.

I think this is a theoretical problem, right? All the current callers pass a pointer to a static pci_ecam_ops struct that does not need to be deallocated.

I see that the next patch uses a pci_ecam_ops struct on the stack, which would definitely cause issues without this patch, so I agree we need something like this; I'm just trying to make the changelog make more sense.

To avoid that extra effort, embed pci_ecam_ops in pci_config_window, instead of just a pointer.

Signed-off-by: Tomasz Nowicki tn@semihalf.com To: Jayachandran C jchandra@broadcom.com

This looks like an editing mistake?

---

arch/arm64/kernel/pci.c | 2 +- drivers/pci/ecam.c | 6 +++--- include/linux/pci-ecam.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index acf3872..981e828 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -183,7 +183,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) return NULL; }

• acpi_pci_root_ops.pci_ops = &ri->cfg->ops->pci_ops;

• acpi_pci_root_ops.pci_ops = &ri->cfg->ops.pci_ops; bus = acpi_pci_root_create(root, &acpi_pci_root_ops, &ri->common, ri->cfg); if (!bus)

diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c index 43ed08d..24242f0 100644 --- a/drivers/pci/ecam.c +++ b/drivers/pci/ecam.c @@ -52,7 +52,7 @@ struct pci_config_window *pci_ecam_create(struct device *dev, return ERR_PTR(-ENOMEM); cfg->parent = dev;

• cfg->ops = ops;

• cfg->ops = *ops; cfg->busr.start = busr->start; cfg->busr.end = busr->end; cfg->busr.flags = IORESOURCE_BUS;

@@ -138,7 +138,7 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn, int where) { struct pci_config_window *cfg = bus->sysdata;

• unsigned int devfn_shift = cfg->ops->bus_shift - 8;

• unsigned int devfn_shift = cfg->ops.bus_shift - 8; unsigned int busn = bus->number; void __iomem *base;

@@ -149,7 +149,7 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn, if (per_bus_mapping) base = cfg->winp[busn]; else

• base = cfg->win + (busn << cfg->ops->bus_shift);
  

• base = cfg->win + (busn << cfg->ops.bus_shift);
  

  return base + (devfn << devfn_shift) + where;

} diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h index 7adad20..0ce2920 100644 --- a/include/linux/pci-ecam.h +++ b/include/linux/pci-ecam.h @@ -39,7 +39,7 @@ struct pci_config_window { struct resource res; struct resource busr; void *priv;

• struct pci_ecam_ops *ops;

• struct pci_ecam_ops ops; union { void __iomem *win; /* 64-bit single mapping */ void __iomem **winp; /* 32-bit per-bus mapping */

-- 1.9.1

On Mon, Aug 08, 2016 at 03:05:38PM +0200, Tomasz Nowicki wrote:

pci_acpi_setup_ecam_mapping() is not really ARM64 specific so move it out of arch/arm64/ directory. In preparation for adding MCFG quirk handling extend pci_acpi_setup_ecam_mapping() functionality to accept custom PCI config accessors (function's argument).

For ARM64 ACPI based PCI host controller we still use pci_generic_ecam_ops.

I'm not sure we gain much by moving pci_acpi_setup_ecam_mapping() from arm64 code to generic code, since nobody else uses it yet. But if you do want to move it, can you do the move (with no other change at all) in one patch, and add the new "ops" argument in a second patch? I just don't want the "ops" change to get lost in the noise of the move.

Signed-off-by: Tomasz Nowicki tn@semihalf.com

arch/arm64/kernel/pci.c | 41 ++--------------------------------------- drivers/acpi/pci_mcfg.c | 40 ++++++++++++++++++++++++++++++++++++++++ include/linux/pci-acpi.h | 3 +++ 3 files changed, 45 insertions(+), 39 deletions(-)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index 981e828..2e7bed4 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -114,44 +114,6 @@ int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) return 0; } -/*

• • Lookup the bus range for the domain in MCFG, and set up config space
• • mapping.
• */

-static struct pci_config_window * -pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root) -{

• struct resource *bus_res = &root->secondary;
• u16 seg = root->segment;
• struct pci_config_window *cfg;
• struct resource cfgres;
• unsigned int bsz;
• /* Use address from _CBA if present, otherwise lookup MCFG */
• if (!root->mcfg_addr)

• root->mcfg_addr = pci_mcfg_lookup(seg, bus_res);
  

• if (!root->mcfg_addr) {

• dev_err(&root->device->dev, "%04x:%pR ECAM region not found\n",
  

• 	seg, bus_res);
  

• return NULL;
  

• }
• bsz = 1 << pci_generic_ecam_ops.bus_shift;
• cfgres.start = root->mcfg_addr + bus_res->start * bsz;
• cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
• cfgres.flags = IORESOURCE_MEM;
• cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res,

• 	      &pci_generic_ecam_ops);
  

• if (IS_ERR(cfg)) {

• dev_err(&root->device->dev, "%04x:%pR error %ld mapping ECAM\n",
  

• 	seg, bus_res, PTR_ERR(cfg));
  

• return NULL;
  

• }
• return cfg;

-}

/* release_info: free resources allocated by init_info */ static void pci_acpi_generic_release_info(struct acpi_pci_root_info *ci) { @@ -177,7 +139,8 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) if (!ri) return NULL;

• ri->cfg = pci_acpi_setup_ecam_mapping(root);

• ri->cfg = pci_acpi_setup_ecam_mapping(root,

• 			      &pci_generic_ecam_ops.pci_ops);
  

  if (!ri->cfg) { kfree(ri); return NULL;

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c index b5b376e..331b560 100644 --- a/drivers/acpi/pci_mcfg.c +++ b/drivers/acpi/pci_mcfg.c @@ -22,6 +22,7 @@ #include <linux/kernel.h> #include <linux/pci.h> #include <linux/pci-acpi.h> +#include <linux/pci-ecam.h> /* Structure to hold entries from the MCFG table */ struct mcfg_entry { @@ -52,6 +53,45 @@ phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res) return 0; } +/*

• • Lookup the bus range for the domain in MCFG, and set up config space
• • mapping.
• */

+struct pci_config_window * +pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root, struct pci_ops *ops) +{

• struct resource *bus_res = &root->secondary;
• u16 seg = root->segment;
• struct pci_config_window *cfg;
• struct resource cfgres;
• struct pci_ecam_ops ecam_ops = {

• 	.bus_shift = 20,
  

• 	.pci_ops = *ops,
  

• };
• /* Use address from _CBA if present, otherwise lookup MCFG */
• if (!root->mcfg_addr)

• root->mcfg_addr = pci_mcfg_lookup(seg, bus_res);
  

• if (!root->mcfg_addr) {

• dev_err(&root->device->dev, "%04x:%pR ECAM region not found\n",
  

• 	seg, bus_res);
  

• return NULL;
  

• }
• cfgres.start = root->mcfg_addr + (bus_res->start << 20);
• cfgres.end = cfgres.start + (resource_size(bus_res) << 20) - 1;
• cfgres.flags = IORESOURCE_MEM;
• cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, &ecam_ops);
• if (IS_ERR(cfg)) {

• dev_err(&root->device->dev, "%04x:%pR error %ld mapping ECAM\n",
  

• 	seg, bus_res, PTR_ERR(cfg));
  

• return NULL;
  

• }
• return cfg;

+}

static __init int pci_mcfg_parse(struct acpi_table_header *header) { struct acpi_table_mcfg *mcfg; diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h index 7d63a66..e9bfe00 100644 --- a/include/linux/pci-acpi.h +++ b/include/linux/pci-acpi.h @@ -26,6 +26,9 @@ extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle); extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource *bus_res); +extern struct pci_config_window * +pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root, struct pci_ops *ops);

static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev) { struct pci_bus *pbus = pdev->bus; -- 1.9.1

On 08.08.2016 17:34, Mark Salter wrote:

On Mon, 2016-08-08 at 15:05 +0200, Tomasz Nowicki wrote:

...

Some platforms may not be fully compliant with generic set of PCI config accessors. For these cases we implement the way to overwrite accessors set. Algorithm traverses available quirk list (static array), matches against <oem_id, oem_table_id, rev, domain, bus number range> and returns pci_config_window structure with fancy PCI config ops. oem_id, oem_table_id and rev come from MCFG table standard header.

It is possible to define custom init call which is responsible for setting up PCI configuration access accordingly to quirk requirements. If custom init call is not defined, use standard pci_acpi_setup_ecam_mapping().

pci_generic_ecam_ops will be used for platforms free from quirks.

Signed-off-by: Tomasz Nowicki tn@semihalf.com Signed-off-by: Dongdong Liu liudongdong3@huawei.com Signed-off-by: Christopher Covington cov@codeaurora.org

---

drivers/pci/host/Makefile | 1 + drivers/pci/host/mcfg-quirks.c | 86 ++++++++++++++++++++++++++++++++++++++++++ drivers/pci/host/mcfg-quirks.h | 20 ++++++++++ include/linux/pci-acpi.h | 2 + 4 files changed, 109 insertions(+) create mode 100644 drivers/pci/host/mcfg-quirks.c create mode 100644 drivers/pci/host/mcfg-quirks.h

diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile index 8843410..500cf78 100644 --- a/drivers/pci/host/Makefile +++ b/drivers/pci/host/Makefile @@ -31,3 +31,4 @@ obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o +obj-$(CONFIG_ACPI_MCFG) += mcfg-quirks.o diff --git a/drivers/pci/host/mcfg-quirks.c b/drivers/pci/host/mcfg-quirks.c new file mode 100644 index 0000000..aa9907b --- /dev/null +++ b/drivers/pci/host/mcfg-quirks.c @@ -0,0 +1,86 @@ +/*

• • Copyright (C) 2016 Semihalf
• • Author: Tomasz Nowicki tn@semihalf.com
• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License, version 2, as
• • published by the Free Software Foundation (the "GPL").
• • This program is distributed in the hope that it will be useful, but
• • WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
• • General Public License version 2 (GPLv2) for more details.
• • You should have received a copy of the GNU General Public License
• • version 2 (GPLv2) along with this source code.
• */

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

+#include "mcfg-quirks.h"

+struct pci_cfg_fixup {

• char oem_id[ACPI_OEM_ID_SIZE + 1];
• char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
• u32 oem_revision;
• struct resource domain_range;
• struct resource bus_range;
• struct pci_ops *ops;
• struct pci_config_window *(*init)(struct acpi_pci_root *root,

• 			  struct pci_ops *ops);
  

+};

+#define MCFG_DOM_RANGE(start, end) DEFINE_RES_NAMED((start), \

• 				((end) - (start) + 1), NULL, 0)
  

+#define MCFG_DOM_ANY MCFG_DOM_RANGE(0x0, 0xffff) +#define MCFG_BUS_RANGE(start, end) DEFINE_RES_NAMED((start), \

• 				((end) - (start) + 1),	\
  

• 				NULL, IORESOURCE_BUS)
  

+#define MCFG_BUS_ANY MCFG_BUS_RANGE(0x0, 0xff)

+static struct pci_cfg_fixup mcfg_quirks[] __initconst = {

                                         ^^^^^^^^^^^^^^

I get section warnings because pci_cfg_fixup_match() is not an init function.

WARNING: vmlinux.o(.text+0x3f6c74): Section mismatch in reference from the function pci_mcfg_match_quirks() to the variable .init.rodata:$d The function pci_mcfg_match_quirks() references the variable __initconst $d. This is often because pci_mcfg_match_quirks lacks a __initconst annotation or the annotation of $d is wrong.

Thanks Mark. I will fix it.

Tomasz

On Mon, Aug 08, 2016 at 03:05:39PM +0200, Tomasz Nowicki wrote:

Some platforms may not be fully compliant with generic set of PCI config accessors. For these cases we implement the way to overwrite accessors set. Algorithm traverses available quirk list (static array), matches against <oem_id, oem_table_id, rev, domain, bus number range> and returns pci_config_window structure with fancy PCI config ops. oem_id, oem_table_id and rev come from MCFG table standard header.

It is possible to define custom init call which is responsible for setting up PCI configuration access accordingly to quirk requirements. If custom init call is not defined, use standard pci_acpi_setup_ecam_mapping().

pci_generic_ecam_ops will be used for platforms free from quirks.

Signed-off-by: Tomasz Nowicki tn@semihalf.com Signed-off-by: Dongdong Liu liudongdong3@huawei.com Signed-off-by: Christopher Covington cov@codeaurora.org

---

drivers/pci/host/Makefile | 1 + drivers/pci/host/mcfg-quirks.c | 86 ++++++++++++++++++++++++++++++++++++++++++ drivers/pci/host/mcfg-quirks.h | 20 ++++++++++ include/linux/pci-acpi.h | 2 + 4 files changed, 109 insertions(+) create mode 100644 drivers/pci/host/mcfg-quirks.c create mode 100644 drivers/pci/host/mcfg-quirks.h

diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile index 8843410..500cf78 100644 --- a/drivers/pci/host/Makefile +++ b/drivers/pci/host/Makefile @@ -31,3 +31,4 @@ obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o +obj-$(CONFIG_ACPI_MCFG) += mcfg-quirks.o diff --git a/drivers/pci/host/mcfg-quirks.c b/drivers/pci/host/mcfg-quirks.c new file mode 100644 index 0000000..aa9907b --- /dev/null +++ b/drivers/pci/host/mcfg-quirks.c @@ -0,0 +1,86 @@ +/*

• • Copyright (C) 2016 Semihalf
• • Author: Tomasz Nowicki tn@semihalf.com
• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License, version 2, as
• • published by the Free Software Foundation (the "GPL").
• • This program is distributed in the hope that it will be useful, but
• • WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
• • General Public License version 2 (GPLv2) for more details.
• • You should have received a copy of the GNU General Public License
• • version 2 (GPLv2) along with this source code.
• */

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

+#include "mcfg-quirks.h"

+struct pci_cfg_fixup {

• char oem_id[ACPI_OEM_ID_SIZE + 1];
• char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
• u32 oem_revision;
• struct resource domain_range;
• struct resource bus_range;
• struct pci_ops *ops;
• struct pci_config_window *(*init)(struct acpi_pci_root *root,

• 			  struct pci_ops *ops);
  

+};

+#define MCFG_DOM_RANGE(start, end) DEFINE_RES_NAMED((start), \

• 				((end) - (start) + 1), NULL, 0)
  

+#define MCFG_DOM_ANY MCFG_DOM_RANGE(0x0, 0xffff) +#define MCFG_BUS_RANGE(start, end) DEFINE_RES_NAMED((start), \

• 				((end) - (start) + 1),	\
  

• 				NULL, IORESOURCE_BUS)
  

+#define MCFG_BUS_ANY MCFG_BUS_RANGE(0x0, 0xff)

+static struct pci_cfg_fixup mcfg_quirks[] __initconst = { +/* { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, pci_ops, init_hook }, */ +};

+static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,

• 		 struct acpi_table_header *mcfg_header)
  

+{

• return (!memcmp(f->oem_id, mcfg_header->oem_id, ACPI_OEM_ID_SIZE) &&

• !memcmp(f->oem_table_id, mcfg_header->oem_table_id,
  

• 	ACPI_OEM_TABLE_ID_SIZE) &&
  

• f->oem_revision == mcfg_header->oem_revision);
  

+}

+struct pci_config_window *pci_mcfg_match_quirks(struct acpi_pci_root *root) +{

• struct resource dom_res = MCFG_DOM_RANGE(root->segment, root->segment);
• struct resource *bus_res = &root->secondary;
• struct pci_cfg_fixup *f = mcfg_quirks;
• struct acpi_table_header *mcfg_header;
• acpi_status status;
• int i;
• status = acpi_get_table(ACPI_SIG_MCFG, 0, &mcfg_header);
• if (ACPI_FAILURE(status))

• return NULL;
  

I remember working out why pci_mcfg_parse() has to cache the data from MCFG, but I don't remember the actual reason... a comment there would have been helpful. Anyway, now I wonder why it's OK to call acpi_get_table(ACPI_SIG_MCFG) again here. I guess I'd be inclined to just cache the OEM info along with the rest of the data so you don't have to get the table again here.

• /*

• * First match against PCI topology <domain:bus> then use OEM ID, OEM
  

• * table ID, and OEM revision from MCFG table standard header.
  

• */
  

• for (i = 0; i < ARRAY_SIZE(mcfg_quirks); i++, f++) {

• if (resource_contains(&f->domain_range, &dom_res) &&
  

•     resource_contains(&f->bus_range, bus_res) &&
  

•     pci_mcfg_fixup_match(f, mcfg_header)) {
  

• 	dev_info(&root->device->dev, "Applying PCI MCFG quirks for %s %s rev: %d\n",
  

• 		 f->oem_id, f->oem_table_id, f->oem_revision);
  

• 	return f->init ? f->init(root, f->ops) :
  

• 		pci_acpi_setup_ecam_mapping(root, f->ops);
  

• }
  

• }
• return pci_acpi_setup_ecam_mapping(root, &pci_generic_ecam_ops.pci_ops);

I think the scope of these MCFG quirks is to:

1) Override (or even fabricate) a MEM resource to be used as ECAM space, and

2) Supply a pci_ecam_ops structure if we need something other than pci_generic_ecam_ops

Today, pci_mcfg_lookup() merely returns a physical address. I didn't chase down where (or whether) that is turned into a struct resource that we put in the iomem tree. I wonder if pci_mcfg_lookup() could be extended to return a resource and a pci_ecam_ops pointer, and handle the quirk application internally, e.g.,

int pci_mcfg_lookup(u16 seg, struct resource *bus_res, struct resource *cfgres, struct pci_ecam_ops **ecam_ops) { struct resource res; struct pci_ecam_ops *ops; struct mcfg_entry *e;

memset(&res, 0, sizeof(res)); res.flags = IORESOURCE_MEM;

e = pci_mcfg_list_search(seg, bus_res); if (e) { res.start = e->addr; res.end = res.start + ...; }

ops = &pci_generic_ecam_ops; ret = pci_mcfg_match_quirks(seg, bus_res, &res, &ops); if (ret) return ret;

*cfgres = res; *ecam_ops = ops; return 0; }

Then the quirks would see any mem resource we discovered via MCFG, and they would see the default pci_ecam_ops, and they could override whatever they needed to.

That would make pci_acpi_setup_ecam_mapping() look something like this:

pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root) { struct resource cfgres; struct pci_ecam_ops *ecam_ops;

ret = pci_mcfg_lookup(root->segment, &root->secondary, &cfgres, &ecam_ops); if (ret) { dev_err(..., "ECAM region not found\n"); return ret; }

cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, ecam_ops); }

+} diff --git a/drivers/pci/host/mcfg-quirks.h b/drivers/pci/host/mcfg-quirks.h new file mode 100644 index 0000000..45cbd16 --- /dev/null +++ b/drivers/pci/host/mcfg-quirks.h @@ -0,0 +1,20 @@ +/*

• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License, version 2, as
• • published by the Free Software Foundation (the "GPL").
• • This program is distributed in the hope that it will be useful, but
• • WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
• • General Public License version 2 (GPLv2) for more details.
• • You should have received a copy of the GNU General Public License
• • version 2 (GPLv2) along with this source code.
• */

+#ifndef __MCFG_QUIRKS_H__ +#define __MCFG_QUIRKS_H__

+/* MCFG quirks initialize call list */

+#endif /* __MCFG_QUIRKS_H__ */ diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h index e9bfe00..28cdce4 100644 --- a/include/linux/pci-acpi.h +++ b/include/linux/pci-acpi.h @@ -25,6 +25,8 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev) extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle); extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource *bus_res); +extern struct pci_config_window * +pci_mcfg_match_quirks(struct acpi_pci_root *root); extern struct pci_config_window * pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root, struct pci_ops *ops); -- 1.9.1

On Mon, Aug 08, 2016 at 03:05:40PM +0200, Tomasz Nowicki wrote:

Since there are platforms which have non-compliant ECAM space we need to override these accessors prior to PCI buses enumeration. In order to do that we call pci_mcfg_match_quirks() to retrieve custom pci_config_window structure. If no correlated quirk on list, use fully ECAM compliant generic PCI config accessors.

Signed-off-by: Tomasz Nowicki tn@semihalf.com

arch/arm64/kernel/pci.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index 2e7bed4..aa734bd 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -139,8 +139,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) if (!ri) return NULL;

• ri->cfg = pci_acpi_setup_ecam_mapping(root,

• 			      &pci_generic_ecam_ops.pci_ops);
  

• ri->cfg = pci_mcfg_match_quirks(root);

I don't really like this part: previously it was obvious that we're setting up ECAM mappings, and now it's not. I think the quirk functionality should be buried somewhere inside; it shouldn't be something this generic code has to worry about.

if (!ri->cfg) { kfree(ri); return NULL; -- 1.9.1

On 05.09.2016 04:34, Bjorn Helgaas wrote:

On Mon, Aug 08, 2016 at 03:05:41PM +0200, Tomasz Nowicki wrote:

...

Add infrastructure to support ThunderX PEM specific PCI configuration space access for ACPI based PCI host controller. This involves:

1. New initialization call thunder_pem_cfg_init() to create configuration

space mapping based on hardcoded range addresses 2. thunder_pem_init() ACPI extension to obtain hardcoded addresses for PEM specific register ranges 3. New quirk entry (for common quirk array) which identifies platform and calls thunder_pem_cfg_init() from [1]

Is it possible to split this up a little bit? I *hope* most quirks aren't as complicated as all this. It'd be nice to make the actual quirk patches as small as possible so they can be easily copied and adapted.

Yes this is a complicated quirk example. Next time I will include a simple one as well.

Also, I will split this patch as you suggested.

Another question below...

...

Signed-off-by: Tomasz Nowicki tn@semihalf.com

drivers/pci/host/mcfg-quirks.c | 7 +++ drivers/pci/host/mcfg-quirks.h | 4 ++ drivers/pci/host/pci-thunder-pem.c | 96 ++++++++++++++++++++++++++++++++------ 3 files changed, 94 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/host/mcfg-quirks.c b/drivers/pci/host/mcfg-quirks.c index aa9907b..2993a72 100644 --- a/drivers/pci/host/mcfg-quirks.c +++ b/drivers/pci/host/mcfg-quirks.c @@ -44,6 +44,13 @@ struct pci_cfg_fixup {

static struct pci_cfg_fixup mcfg_quirks[] __initconst = { /* { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, pci_ops, init_hook }, */ +#ifdef CONFIG_PCI_HOST_THUNDER_PEM

• /* Pass2.0 */
• { "CAVIUM", "THUNDERX", 1, MCFG_DOM_RANGE(4, 9), MCFG_BUS_ANY, NULL,

•  thunder_pem_cfg_init },
  

• { "CAVIUM", "THUNDERX", 1, MCFG_DOM_RANGE(14, 19), MCFG_BUS_ANY, NULL,

•  thunder_pem_cfg_init },
  

+#endif };

static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f, diff --git a/drivers/pci/host/mcfg-quirks.h b/drivers/pci/host/mcfg-quirks.h index 45cbd16..411c667 100644 --- a/drivers/pci/host/mcfg-quirks.h +++ b/drivers/pci/host/mcfg-quirks.h @@ -16,5 +16,9 @@ #define __MCFG_QUIRKS_H__

/* MCFG quirks initialize call list */ +#ifdef CONFIG_PCI_HOST_THUNDER_PEM +struct pci_config_window * +thunder_pem_cfg_init(struct acpi_pci_root *root, struct pci_ops *ops); +#endif

#endif /* __MCFG_QUIRKS_H__ */ diff --git a/drivers/pci/host/pci-thunder-pem.c b/drivers/pci/host/pci-thunder-pem.c index 6abaf80..3f06e49 100644 --- a/drivers/pci/host/pci-thunder-pem.c +++ b/drivers/pci/host/pci-thunder-pem.c @@ -18,9 +18,12 @@ #include <linux/init.h> #include <linux/of_address.h> #include <linux/of_pci.h> +#include <linux/pci-acpi.h> #include <linux/pci-ecam.h> #include <linux/platform_device.h>

+#include "mcfg-quirks.h"

#define PEM_CFG_WR 0x28 #define PEM_CFG_RD 0x30

@@ -284,6 +287,37 @@ static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn, return pci_generic_config_write(bus, devfn, where, size, val); }

+#ifdef CONFIG_ACPI

+static struct resource thunder_pem_reg_res[] = {

• [4] = DEFINE_RES_MEM(0x87e0c0000000UL, SZ_16M),
• [5] = DEFINE_RES_MEM(0x87e0c1000000UL, SZ_16M),
• [6] = DEFINE_RES_MEM(0x87e0c2000000UL, SZ_16M),
• [7] = DEFINE_RES_MEM(0x87e0c3000000UL, SZ_16M),
• [8] = DEFINE_RES_MEM(0x87e0c4000000UL, SZ_16M),
• [9] = DEFINE_RES_MEM(0x87e0c5000000UL, SZ_16M),
• [14] = DEFINE_RES_MEM(0x97e0c0000000UL, SZ_16M),
• [15] = DEFINE_RES_MEM(0x97e0c1000000UL, SZ_16M),
• [16] = DEFINE_RES_MEM(0x97e0c2000000UL, SZ_16M),
• [17] = DEFINE_RES_MEM(0x97e0c3000000UL, SZ_16M),
• [18] = DEFINE_RES_MEM(0x97e0c4000000UL, SZ_16M),
• [19] = DEFINE_RES_MEM(0x97e0c5000000UL, SZ_16M),

+};

+static struct resource *thunder_pem_acpi_res(struct pci_config_window *cfg) +{

• struct acpi_device *adev = to_acpi_device(cfg->parent);
• struct acpi_pci_root *root = acpi_driver_data(adev);
• return &thunder_pem_reg_res[root->segment];

+} +#else +static struct resource *thunder_pem_acpi_res(struct pci_config_window *cfg) +{

• return NULL;

+} +#endif

static int thunder_pem_init(struct pci_config_window *cfg) { struct device *dev = cfg->parent; @@ -292,24 +326,24 @@ static int thunder_pem_init(struct pci_config_window *cfg) struct thunder_pem_pci *pem_pci; struct platform_device *pdev;

• /* Only OF support for now */

• if (!dev->of_node)

• return -EINVAL;
  

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

• pdev = to_platform_device(dev);

• /*

• * The second register range is the PEM bridge to the PCIe
  

• * bus.  It has a different config access method than those
  

• * devices behind the bridge.
  

• */
  

• res_pem = platform_get_resource(pdev, IORESOURCE_MEM, 1);

• if (acpi_disabled) {

• pdev = to_platform_device(dev);
  

• /*
  

•  * The second register range is the PEM bridge to the PCIe
  

•  * bus.  It has a different config access method than those
  

•  * devices behind the bridge.
  

•  */
  

• res_pem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
  

• } else {

• res_pem = thunder_pem_acpi_res(cfg);
  

• } if (!res_pem) {

• dev_err(dev, "missing \"reg[1]\"property\n");
  

• dev_err(dev, "missing configuration region\n");
  

  return -EINVAL; }

@@ -360,3 +394,39 @@ static struct platform_driver thunder_pem_driver = { .probe = thunder_pem_probe, }; builtin_platform_driver(thunder_pem_driver);

+#ifdef CONFIG_ACPI

+static struct resource thunder_pem_cfg_res[] = {

• [4] = DEFINE_RES_MEM(0x88001f000000UL, 0x39 * SZ_16M),
• [5] = DEFINE_RES_MEM(0x884057000000UL, 0x39 * SZ_16M),
• [6] = DEFINE_RES_MEM(0x88808f000000UL, 0x39 * SZ_16M),
• [7] = DEFINE_RES_MEM(0x89001f000000UL, 0x39 * SZ_16M),
• [8] = DEFINE_RES_MEM(0x894057000000UL, 0x39 * SZ_16M),
• [9] = DEFINE_RES_MEM(0x89808f000000UL, 0x39 * SZ_16M),
• [14] = DEFINE_RES_MEM(0x98001f000000UL, 0x39 * SZ_16M),
• [15] = DEFINE_RES_MEM(0x984057000000UL, 0x39 * SZ_16M),
• [16] = DEFINE_RES_MEM(0x98808f000000UL, 0x39 * SZ_16M),
• [17] = DEFINE_RES_MEM(0x99001f000000UL, 0x39 * SZ_16M),
• [18] = DEFINE_RES_MEM(0x994057000000UL, 0x39 * SZ_16M),
• [19] = DEFINE_RES_MEM(0x99808f000000UL, 0x39 * SZ_16M),

+};

+struct pci_config_window * +thunder_pem_cfg_init(struct acpi_pci_root *root, struct pci_ops *ops) +{

• struct resource *bus_res = &root->secondary;
• u16 seg = root->segment;
• struct pci_config_window *cfg;
• cfg = pci_ecam_create(&root->device->dev, &thunder_pem_cfg_res[seg],

• 	      bus_res, &pci_thunder_pem_ops);
  

What happens when root->segment is not one of the values defined in thunder_pem_cfg_res[], say "0"? I *think* pci_ecam_create() will see cfgres as all zeros, and the request_resource_conflict() will probably fail. That doesn't seem like a very graceful way to report a firmware error.

Right, I will fix it. Thanks.

Tomasz