[RFC PATCH v4 0/5] ECAM quirks handling for ARM64 platforms

[RFC PATCH v4 0/5] ECAM quirks handling for ARM64 platforms

[Tomasz Nowicki]

Quirk handling relies on an idea of matching MCFG OEM ID, TABLE ID and
revision (the ones from standard header of MCFG table).

Static array is used to keep quirk entries. Each entry consist of
metioned MCFG IDs along with custom pci_ops structure and initialization call.

As an example, last patch presents quirk handling mechanism usage for
ThunderX PEM driver.

Tomasz Nowicki (5):
  PCI: Embed pci_ecam_ops in pci_config_window structure
  PCI/ACPI: Move ACPI ECAM mapping to generic MCFG driver
  PCI: Check platform specific ECAM quirks
  ARM64/PCI: Start using quirks handling for ACPI based PCI host
    controller
  PCI: thunder: Add ThunderX PEM MCFG quirk to the list

 arch/arm64/kernel/pci.c            | 42 +----------------
 drivers/acpi/pci_mcfg.c            | 40 ++++++++++++++++
 drivers/pci/ecam.c                 |  6 +--
 drivers/pci/host/Makefile          |  1 +
 drivers/pci/host/mcfg-quirks.c     | 95 +++++++++++++++++++++++++++++++++++++
 drivers/pci/host/mcfg-quirks.h     | 24 ++++++++++
 drivers/pci/host/pci-thunder-pem.c | 96 ++++++++++++++++++++++++++++++++------
 include/linux/pci-acpi.h           |  5 ++
 include/linux/pci-ecam.h           |  2 +-
 9 files changed, 254 insertions(+), 57 deletions(-)
 create mode 100644 drivers/pci/host/mcfg-quirks.c
 create mode 100644 drivers/pci/host/mcfg-quirks.h

-- 
1.9.1

[RFC PATCH v4 1/5] PCI: Embed pci_ecam_ops in pci_config_window structure

[Tomasz Nowicki]

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.

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>
---
 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 94cd43c..cb4dee7 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -196,7 +196,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

[RFC PATCH v4 2/5] PCI/ACPI: Move ACPI ECAM mapping to generic MCFG driver

[Tomasz Nowicki]

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.

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 cb4dee7..addf00c 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -127,44 +127,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)
 {
@@ -190,7 +152,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

[RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks

[Tomasz Nowicki]

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 config access accordingly to quirk requirements.
If no custom init call 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 | 88 ++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/host/mcfg-quirks.h | 20 ++++++++++
 include/linux/pci-acpi.h       |  2 +
 4 files changed, 111 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 5bc0af2..e3d55a0 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -30,3 +30,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..fb2b184
--- /dev/null
+++ b/drivers/pci/host/mcfg-quirks.c
@@ -0,0 +1,88 @@
+/*
+ * 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;
+	char *oem_table_id;
+	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_qurks[] __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)
+{
+	int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
+	int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
+
+	return (!strncmp(f->oem_id, mcfg_header->oem_id, olen) &&
+		!strncmp(f->oem_table_id, mcfg_header->oem_table_id, tlen) &&
+		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_qurks;
+	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;
+
+	/*
+	 * 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_qurks); i++, f++) {
+		if (resource_contains(&f->domain_range, &dom_res) &&
+		    resource_contains(&f->bus_range, bus_res) &&
+		    pci_mcfg_fixup_match(f, mcfg_header)) {
+			pr_info("Handling %s %s r%d PCI MCFG quirks\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);
+}
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

[RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks

[Christopher Covington]

Hi Tomasz,

On 06/28/2016 03:54 AM, Tomasz Nowicki wrote:

> diff --git a/drivers/pci/host/mcfg-quirks.c b/drivers/pci/host/mcfg-quirks.c
> new file mode 100644
> index 0000000..fb2b184
> --- /dev/null
> +++ b/drivers/pci/host/mcfg-quirks.c
> @@ -0,0 +1,88 @@

> +static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
> +				 struct acpi_table_header *mcfg_header)
> +{
> +	int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
> +	int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
> +
> +	return (!strncmp(f->oem_id, mcfg_header->oem_id, olen) &&
> +		!strncmp(f->oem_table_id, mcfg_header->oem_table_id, tlen) &&
> +		f->oem_revision == mcfg_header->oem_revision);
> +}

Ard's comments on v3 included:

"... exact OEM table/rev id matches ..."
"... substring match ... out of the question ..."

I originally advocated the substring match approach because
space-padding the input strings was unfamiliar. But given that some
vendors have a "PLAT    " then "PLAT2   " naming scheme, where the
former needs quirks and the latter (hopefully) doesn't, I agree with Ard
and think space-padded inputs is the better way to go. Sorry for the
lack of foresight.

(I'm happy to rip it out, test, and communicate the delta however you'd
prefer--just let me know.)

Regards,
Cov

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks

[Duc Dang]

On Tue, Jun 28, 2016 at 6:04 AM, Christopher Covington
<cov@codeaurora.org> wrote:
> Hi Tomasz,
>
> On 06/28/2016 03:54 AM, Tomasz Nowicki wrote:
>
>> diff --git a/drivers/pci/host/mcfg-quirks.c b/drivers/pci/host/mcfg-quirks.c
>> new file mode 100644
>> index 0000000..fb2b184
>> --- /dev/null
>> +++ b/drivers/pci/host/mcfg-quirks.c
>> @@ -0,0 +1,88 @@
>
>> +static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
>> +                              struct acpi_table_header *mcfg_header)
>> +{
>> +     int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
>> +     int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
>> +
>> +     return (!strncmp(f->oem_id, mcfg_header->oem_id, olen) &&
>> +             !strncmp(f->oem_table_id, mcfg_header->oem_table_id, tlen) &&
>> +             f->oem_revision == mcfg_header->oem_revision);
>> +}
>
> Ard's comments on v3 included:
>
> "... exact OEM table/rev id matches ..."
> "... substring match ... out of the question ..."
>
> I originally advocated the substring match approach because
> space-padding the input strings was unfamiliar. But given that some
> vendors have a "PLAT    " then "PLAT2   " naming scheme, where the
> former needs quirks and the latter (hopefully) doesn't, I agree with Ard
> and think space-padded inputs is the better way to go. Sorry for the
> lack of foresight.

I think having OEM Table ID as "PLAT   " and then "PLAT2  " (the the
next version of the SoC)
is common. So yes, matching full string is better as we can use "PLAT2  "
in MCFG table and not worry about the "PLAT" sub-string match causes the quirk
to be applied unintentionally.

>
> (I'm happy to rip it out, test, and communicate the delta however you'd
> prefer--just let me know.)
>
> Regards,
> Cov
>
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
Regards,
Duc Dang.

[RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks

[Tomasz Nowicki]

On 28.06.2016 18:12, Duc Dang wrote:
> On Tue, Jun 28, 2016 at 6:04 AM, Christopher Covington
> <cov@codeaurora.org> wrote:
>> Hi Tomasz,
>>
>> On 06/28/2016 03:54 AM, Tomasz Nowicki wrote:
>>
>>> diff --git a/drivers/pci/host/mcfg-quirks.c b/drivers/pci/host/mcfg-quirks.c
>>> new file mode 100644
>>> index 0000000..fb2b184
>>> --- /dev/null
>>> +++ b/drivers/pci/host/mcfg-quirks.c
>>> @@ -0,0 +1,88 @@
>>
>>> +static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
>>> +                              struct acpi_table_header *mcfg_header)
>>> +{
>>> +     int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
>>> +     int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
>>> +
>>> +     return (!strncmp(f->oem_id, mcfg_header->oem_id, olen) &&
>>> +             !strncmp(f->oem_table_id, mcfg_header->oem_table_id, tlen) &&
>>> +             f->oem_revision == mcfg_header->oem_revision);
>>> +}
>>
>> Ard's comments on v3 included:
>>
>> "... exact OEM table/rev id matches ..."
>> "... substring match ... out of the question ..."
>>
>> I originally advocated the substring match approach because
>> space-padding the input strings was unfamiliar. But given that some
>> vendors have a "PLAT    " then "PLAT2   " naming scheme, where the
>> former needs quirks and the latter (hopefully) doesn't, I agree with Ard
>> and think space-padded inputs is the better way to go. Sorry for the
>> lack of foresight.
>
> I think having OEM Table ID as "PLAT   " and then "PLAT2  " (the the
> next version of the SoC)
> is common. So yes, matching full string is better as we can use "PLAT2  "
> in MCFG table and not worry about the "PLAT" sub-string match causes the quirk
> to be applied unintentionally.
>

Sorry, I forgot to address Ard's comment.

Note that platforms already shipped where OEM string has no padding will 
have change the firmware or add 0 padding to our quirk array IDs.
So how about:

static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
				 struct acpi_table_header *mcfg_header)
{
	int olen = strnlen(mcfg_header->oem_id, ACPI_OEM_ID_SIZE);
	int tlen = strnlen(mcfg_header->oem_table_id, ACPI_OEM_TABLE_ID_SIZE);

	if (olen != strlen(f->oem_id) || tlen != strlen(f->oem_table_id))
		return false;

	return (!strncmp(f->oem_id, mcfg_header->oem_id, olen) &&
		!strncmp(f->oem_table_id, mcfg_header->oem_table_id, tlen) &&
		f->oem_revision == mcfg_header->oem_revision);
}

This should work for all cases:
1. "PLAT"
2. "PLAT    " padding
3. No need to change existing firmware

Tomasz

[RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks

[Christopher Covington]

Hi Tomasz,

On 06/29/2016 06:48 AM, Tomasz Nowicki wrote:
> On 28.06.2016 18:12, Duc Dang wrote:
>> On Tue, Jun 28, 2016 at 6:04 AM, Christopher Covington
>> <cov@codeaurora.org> wrote:
>>> Hi Tomasz,

>>> Ard's comments on v3 included:
>>>
>>> "... exact OEM table/rev id matches ..."
>>> "... substring match ... out of the question ..."

Digging through the archives I see Jon Master commented earlier to "be
careful with substring match".

>> I think having OEM Table ID as "PLAT " and then "PLAT2 " (the the 
>> next version of the SoC) is common. So yes, matching full string is
>> better as we can use "PLAT2 " in MCFG table and not worry about the
>> "PLAT" sub-string match causes the quirk to be applied
>> unintentionally.

> Note that platforms already shipped where OEM string has no padding will

I'm confused by this statement. OEMID is defined as 6 bytes long and OEM
Table ID as 8 bytes long in the ACPI specification. As far as I can
tell, if your string isn't exactly that long, padding up to that length
is required.

> have change the firmware or add 0 padding to our quirk array IDs.

The fixed 6 or 8 character string compare, as used v2 of this patchset,
will be compatible with existing firmware as best I can tell. Adding
padding to the quirk array IDs is exactly what I'm suggesting, although
all the strings I've seen are space padded rather than null padded.

Matches:
{"APM   ", "XGENE   ", 1}
{"CAVIUM", "THUNDERX", 1}
{"HISI  ", "HISI-D02", 1}
{"HISI  ", "HISI-D03", 1}
{"QCOM  ", "QDF2432 ", 1}

Given the above tuples, won't accidentally match:
(guessing at possible future ids)
{"APM   ", "XGENEi  ", 1}
{"CAVIUM", "THUNDERX", i} i != 1
{"CAVIUM", "THUNDERi", 1}
{"CAVIUM", "THUNDRXi", 1}
{"HISI  ", "HISI-D0i", 1} i != 2 && i != 3
{"QCOM  ", "QDF24ij ", 1} i != 3 && j != 2

References for APM, HiSilicon IDs:
https://lists.linaro.org/pipermail/linaro-acpi/2016-June/007108.html
https://lists.linaro.org/pipermail/linaro-acpi/2016-June/007043.html

Thanks,
Cov

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks

[Tomasz Nowicki]

On 29.06.2016 15:34, Christopher Covington wrote:
> I'm confused by this statement. OEMID is defined as 6 bytes long and OEM
> Table ID as 8 bytes long in the ACPI specification. As far as I can
> tell, if your string isn't exactly that long, padding up to that length
> is required.

Well, I cannot find that requirement in ACPI spec. but I might missed 
something.

I dumped my x86 machine ACPI tables and here is an example of MCFG:

$ cat mcfg.dsl
/*
  * Intel ACPI Component Architecture
  * AML/ASL+ Disassembler version 20160108-64
  * Copyright (c) 2000 - 2016 Intel Corporation
  *
  * Disassembly of mcfg.dat, Wed Jun 29 15:48:16 2016
  *
  * ACPI Data Table [MCFG]
  *
  * Format: [HexOffset DecimalOffset ByteLength]  FieldName : FieldValue
  */

[000h 0000   4]                    Signature : "MCFG"    [Memory Mapped 
Configuration table]
[004h 0004   4]                 Table Length : 0000003C
[008h 0008   1]                     Revision : 01
[009h 0009   1]                     Checksum : A9
[00Ah 0010   6]                       Oem ID : "ALASKA"
[010h 0016   8]                 Oem Table ID : "A M I"
[018h 0024   4]                 Oem Revision : 01072009
[01Ch 0028   4]              Asl Compiler ID : "MSFT"
[020h 0032   4]        Asl Compiler Revision : 00000097

[024h 0036   8]                     Reserved : 0000000000000000

[02Ch 0044   8]                 Base Address : 00000000F8000000
[034h 0052   2]         Segment Group Number : 0000
[036h 0054   1]             Start Bus Number : 00
[037h 0055   1]               End Bus Number : 3F
[038h 0056   4]                     Reserved : 00000000

Raw Table Data: Length 60 (0x3C)

00000000  4d 43 46 47 3c 00 00 00  01 a9 41 4c 41 53 4b 41 
|MCFG<.....ALASKA|
00000010  41 20 4d 20 49 00 00 00  09 20 07 01 4d 53 46 54  |A M I.... 
..MSFT|
00000020  97 00 00 00 00 00 00 00  00 00 00 00 00 00 00 f8 
|................|
00000030  00 00 00 00 00 00 00 3f  00 00 00 00              |.......?....|

So in this example I have OEM table ID "A M I" 6 character long and 0 
padding.

Tomasz

[RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks

[Ard Biesheuvel]

On 29 June 2016 at 15:52, Tomasz Nowicki <tn@semihalf.com> wrote:
> On 29.06.2016 15:34, Christopher Covington wrote:
>>
>> I'm confused by this statement. OEMID is defined as 6 bytes long and OEM
>> Table ID as 8 bytes long in the ACPI specification. As far as I can
>> tell, if your string isn't exactly that long, padding up to that length
>> is required.
>
>
> Well, I cannot find that requirement in ACPI spec. but I might missed
> something.
>

This has *nothing* to do with the ACPI spec.

There exist a couple of machines that have known quirks. Those
machines have certain 6 or 8 byte long strings in their MCFG tables
that we can match against. If any of those fields contains spaces,
whether you call that padding or not is totally irrelevant. The same
applies to NUL bytes.

So for a 6 byte field, our table contains the six characters that we
know is in the quirky hardware's table.
The same for an 8 byte field.

That means there is no need for strnlen, strncmp() with variable size
etc. Each match can be implemented using memcmp() [since the table
fields are not NUL terminated] with a constant argument for the length
parameter.

Regards,
Ard.


> I dumped my x86 machine ACPI tables and here is an example of MCFG:
>
> $ cat mcfg.dsl
> /*
>  * Intel ACPI Component Architecture
>  * AML/ASL+ Disassembler version 20160108-64
>  * Copyright (c) 2000 - 2016 Intel Corporation
>  *
>  * Disassembly of mcfg.dat, Wed Jun 29 15:48:16 2016
>  *
>  * ACPI Data Table [MCFG]
>  *
>  * Format: [HexOffset DecimalOffset ByteLength]  FieldName : FieldValue
>  */
>
> [000h 0000   4]                    Signature : "MCFG"    [Memory Mapped
> Configuration table]
> [004h 0004   4]                 Table Length : 0000003C
> [008h 0008   1]                     Revision : 01
> [009h 0009   1]                     Checksum : A9
> [00Ah 0010   6]                       Oem ID : "ALASKA"
> [010h 0016   8]                 Oem Table ID : "A M I"
> [018h 0024   4]                 Oem Revision : 01072009
> [01Ch 0028   4]              Asl Compiler ID : "MSFT"
> [020h 0032   4]        Asl Compiler Revision : 00000097
>
> [024h 0036   8]                     Reserved : 0000000000000000
>
> [02Ch 0044   8]                 Base Address : 00000000F8000000
> [034h 0052   2]         Segment Group Number : 0000
> [036h 0054   1]             Start Bus Number : 00
> [037h 0055   1]               End Bus Number : 3F
> [038h 0056   4]                     Reserved : 00000000
>
> Raw Table Data: Length 60 (0x3C)
>
> 00000000  4d 43 46 47 3c 00 00 00  01 a9 41 4c 41 53 4b 41
> |MCFG<.....ALASKA|
> 00000010  41 20 4d 20 49 00 00 00  09 20 07 01 4d 53 46 54  |A M I....
> ..MSFT|
> 00000020  97 00 00 00 00 00 00 00  00 00 00 00 00 00 00 f8
> |................|
> 00000030  00 00 00 00 00 00 00 3f  00 00 00 00              |.......?....|
>
> So in this example I have OEM table ID "A M I" 6 character long and 0
> padding.
>
> Tomasz
>

[RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks

[Jeffrey Hugo]

On 6/29/2016 7:52 AM, Tomasz Nowicki wrote:
> On 29.06.2016 15:34, Christopher Covington wrote:
>> I'm confused by this statement. OEMID is defined as 6 bytes long and OEM
>> Table ID as 8 bytes long in the ACPI specification. As far as I can
>> tell, if your string isn't exactly that long, padding up to that length
>> is required.
>
> Well, I cannot find that requirement in ACPI spec. but I might missed
> something.

Have a look at table 5-29 in Section 5.2.6 which starts on page 114 of 
ACPI 6.1

>
> I dumped my x86 machine ACPI tables and here is an example of MCFG:
>
> $ cat mcfg.dsl
> /*
>  * Intel ACPI Component Architecture
>  * AML/ASL+ Disassembler version 20160108-64
>  * Copyright (c) 2000 - 2016 Intel Corporation
>  *
>  * Disassembly of mcfg.dat, Wed Jun 29 15:48:16 2016
>  *
>  * ACPI Data Table [MCFG]
>  *
>  * Format: [HexOffset DecimalOffset ByteLength]  FieldName : FieldValue
>  */
>
> [000h 0000   4]                    Signature : "MCFG"    [Memory Mapped
> Configuration table]
> [004h 0004   4]                 Table Length : 0000003C
> [008h 0008   1]                     Revision : 01
> [009h 0009   1]                     Checksum : A9
> [00Ah 0010   6]                       Oem ID : "ALASKA"
> [010h 0016   8]                 Oem Table ID : "A M I"
> [018h 0024   4]                 Oem Revision : 01072009
> [01Ch 0028   4]              Asl Compiler ID : "MSFT"
> [020h 0032   4]        Asl Compiler Revision : 00000097
>
> [024h 0036   8]                     Reserved : 0000000000000000
>
> [02Ch 0044   8]                 Base Address : 00000000F8000000
> [034h 0052   2]         Segment Group Number : 0000
> [036h 0054   1]             Start Bus Number : 00
> [037h 0055   1]               End Bus Number : 3F
> [038h 0056   4]                     Reserved : 00000000
>
> Raw Table Data: Length 60 (0x3C)
>
> 00000000  4d 43 46 47 3c 00 00 00  01 a9 41 4c 41 53 4b 41
> |MCFG<.....ALASKA|
> 00000010  41 20 4d 20 49 00 00 00  09 20 07 01 4d 53 46 54  |A M I....
> ..MSFT|
> 00000020  97 00 00 00 00 00 00 00  00 00 00 00 00 00 00 f8
> |................|
> 00000030  00 00 00 00 00 00 00 3f  00 00 00 00              |.......?....|
>
> So in this example I have OEM table ID "A M I" 6 character long and 0
> padding.
>
> Tomasz
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


-- 
Jeffrey Hugo
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

[RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks

[Ard Biesheuvel]

On 29 June 2016 at 15:34, Christopher Covington <cov@codeaurora.org> wrote:
> Hi Tomasz,
>
> On 06/29/2016 06:48 AM, Tomasz Nowicki wrote:
>> On 28.06.2016 18:12, Duc Dang wrote:
>>> On Tue, Jun 28, 2016 at 6:04 AM, Christopher Covington
>>> <cov@codeaurora.org> wrote:
>>>> Hi Tomasz,
>
>>>> Ard's comments on v3 included:
>>>>
>>>> "... exact OEM table/rev id matches ..."
>>>> "... substring match ... out of the question ..."
>
> Digging through the archives I see Jon Master commented earlier to "be
> careful with substring match".
>
>>> I think having OEM Table ID as "PLAT " and then "PLAT2 " (the the
>>> next version of the SoC) is common. So yes, matching full string is
>>> better as we can use "PLAT2 " in MCFG table and not worry about the
>>> "PLAT" sub-string match causes the quirk to be applied
>>> unintentionally.
>
>> Note that platforms already shipped where OEM string has no padding will
>
> I'm confused by this statement. OEMID is defined as 6 bytes long and OEM
> Table ID as 8 bytes long in the ACPI specification. As far as I can
> tell, if your string isn't exactly that long, padding up to that length
> is required.
>
>> have change the firmware or add 0 padding to our quirk array IDs.
>
> The fixed 6 or 8 character string compare, as used v2 of this patchset,
> will be compatible with existing firmware as best I can tell. Adding
> padding to the quirk array IDs is exactly what I'm suggesting, although
> all the strings I've seen are space padded rather than null padded.
>

I don't think any interpretation of the 6 or 8 byte wide OEM fields is
necessary to be able to match it against a list of known values as
used by the quirky platforms. We need an exact match against whatever
we know is in the table of an affected system, and whether a space
qualifies as padding or as a character is irrelevant.

> Matches:
> {"APM   ", "XGENE   ", 1}
> {"CAVIUM", "THUNDERX", 1}
> {"HISI  ", "HISI-D02", 1}
> {"HISI  ", "HISI-D03", 1}
> {"QCOM  ", "QDF2432 ", 1}
>

I would not mind listing these as

{ { 'A','P','M',' ',' ',' ',' '}, {'X','G','E','N','E',' ',' ',' '}, 1}
...

just to stress that we are not dealing with C strings (and to avoid
having to deal with the implicit NUL terminator).
That also means memcmp() with a fixed length is the most appropriate
to perform the comparison

> Given the above tuples, won't accidentally match:
> (guessing at possible future ids)
> {"APM   ", "XGENEi  ", 1}
> {"CAVIUM", "THUNDERX", i} i != 1
> {"CAVIUM", "THUNDERi", 1}
> {"CAVIUM", "THUNDRXi", 1}
> {"HISI  ", "HISI-D0i", 1} i != 2 && i != 3
> {"QCOM  ", "QDF24ij ", 1} i != 3 && j != 2
>
> References for APM, HiSilicon IDs:
> https://lists.linaro.org/pipermail/linaro-acpi/2016-June/007108.html
> https://lists.linaro.org/pipermail/linaro-acpi/2016-June/007043.html
>
> Thanks,
> Cov
>
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

[RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks

[Robert Richter]

On 29.06.16 15:56:50, Ard Biesheuvel wrote:
> On 29 June 2016 at 15:34, Christopher Covington <cov@codeaurora.org> wrote:
> > Hi Tomasz,
> >
> > On 06/29/2016 06:48 AM, Tomasz Nowicki wrote:
> >> On 28.06.2016 18:12, Duc Dang wrote:
> >>> On Tue, Jun 28, 2016 at 6:04 AM, Christopher Covington
> >>> <cov@codeaurora.org> wrote:
> >>>> Hi Tomasz,
> >
> >>>> Ard's comments on v3 included:
> >>>>
> >>>> "... exact OEM table/rev id matches ..."
> >>>> "... substring match ... out of the question ..."
> >
> > Digging through the archives I see Jon Master commented earlier to "be
> > careful with substring match".
> >
> >>> I think having OEM Table ID as "PLAT " and then "PLAT2 " (the the
> >>> next version of the SoC) is common. So yes, matching full string is
> >>> better as we can use "PLAT2 " in MCFG table and not worry about the
> >>> "PLAT" sub-string match causes the quirk to be applied
> >>> unintentionally.
> >
> >> Note that platforms already shipped where OEM string has no padding will
> >
> > I'm confused by this statement. OEMID is defined as 6 bytes long and OEM
> > Table ID as 8 bytes long in the ACPI specification. As far as I can
> > tell, if your string isn't exactly that long, padding up to that length
> > is required.
> >
> >> have change the firmware or add 0 padding to our quirk array IDs.
> >
> > The fixed 6 or 8 character string compare, as used v2 of this patchset,
> > will be compatible with existing firmware as best I can tell. Adding
> > padding to the quirk array IDs is exactly what I'm suggesting, although
> > all the strings I've seen are space padded rather than null padded.
> >
> 
> I don't think any interpretation of the 6 or 8 byte wide OEM fields is
> necessary to be able to match it against a list of known values as
> used by the quirky platforms. We need an exact match against whatever
> we know is in the table of an affected system, and whether a space
> qualifies as padding or as a character is irrelevant.
> 
> > Matches:
> > {"APM   ", "XGENE   ", 1}
> > {"CAVIUM", "THUNDERX", 1}
> > {"HISI  ", "HISI-D02", 1}
> > {"HISI  ", "HISI-D03", 1}
> > {"QCOM  ", "QDF2432 ", 1}
> >
> 
> I would not mind listing these as
> 
> { { 'A','P','M',' ',' ',' ',' '}, {'X','G','E','N','E',' ',' ',' '}, 1}
> ...
> 
> just to stress that we are not dealing with C strings (and to avoid
> having to deal with the implicit NUL terminator).
> That also means memcmp() with a fixed length is the most appropriate
> to perform the comparison

I still would go with memcmp but have the char arrays null terminated
in addition. This first makes string handling easier, and fixes some
unterminated %s printfs bugs in the code.

Thus, I would prefer to go with:

struct pci_cfg_fixup {
        char oem_id[ACPI_OEM_ID_SIZE + 1];
        char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
	...

static struct pci_cfg_quirks mcfg_qurks[] __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, ...

This is also no "pain in the eyes". :)

If there are zero bytes in then just use \0, e.g.:

 { "foo\0\0\0", "foobar\0\0", ... }

For comparisation still use memcmp accordingly:

 memcmp(..., ACPI_OEM_ID_SIZE);
 memcmp(..., ACPI_OEM_TABLE_ID_SIZE);

The following would be fixed too as strings are now null terminated:

	pr_info("Handling %s %s r%d PCI MCFG quirks\n",
		f->oem_id, f->oem_table_id, f->oem_revision);

Btw, use dev_info(&root->device->dev, ...) here for pr_info() and
modify message text, e.g.:

 acpi PNP0A08:04: Applying PCI MCFG quirks for CAVIUM THUNDERX rev: 1

And, we should support some sort of MCFG_OEM_REVISION_ANY to move the
rev handling optional to pci_cfg_fixup::init().

Plus one spelling fix: mcfg_qurks -> mcfg_quirks 

Thanks,

-Robert

> 
> > Given the above tuples, won't accidentally match:
> > (guessing at possible future ids)
> > {"APM   ", "XGENEi  ", 1}
> > {"CAVIUM", "THUNDERX", i} i != 1
> > {"CAVIUM", "THUNDERi", 1}
> > {"CAVIUM", "THUNDRXi", 1}
> > {"HISI  ", "HISI-D0i", 1} i != 2 && i != 3
> > {"QCOM  ", "QDF24ij ", 1} i != 3 && j != 2
> >
> > References for APM, HiSilicon IDs:
> > https://lists.linaro.org/pipermail/linaro-acpi/2016-June/007108.html
> > https://lists.linaro.org/pipermail/linaro-acpi/2016-June/007043.html
> >
> > Thanks,
> > Cov
> >
> > --
> > Qualcomm Innovation Center, Inc.
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > a Linux Foundation Collaborative Project

[RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks

[Ard Biesheuvel]

On 22 July 2016 at 13:38, Robert Richter <rric@kernel.org> wrote:
> On 29.06.16 15:56:50, Ard Biesheuvel wrote:
>> On 29 June 2016 at 15:34, Christopher Covington <cov@codeaurora.org> wrote:
>> > Hi Tomasz,
>> >
>> > On 06/29/2016 06:48 AM, Tomasz Nowicki wrote:
>> >> On 28.06.2016 18:12, Duc Dang wrote:
>> >>> On Tue, Jun 28, 2016 at 6:04 AM, Christopher Covington
>> >>> <cov@codeaurora.org> wrote:
>> >>>> Hi Tomasz,
>> >
>> >>>> Ard's comments on v3 included:
>> >>>>
>> >>>> "... exact OEM table/rev id matches ..."
>> >>>> "... substring match ... out of the question ..."
>> >
>> > Digging through the archives I see Jon Master commented earlier to "be
>> > careful with substring match".
>> >
>> >>> I think having OEM Table ID as "PLAT " and then "PLAT2 " (the the
>> >>> next version of the SoC) is common. So yes, matching full string is
>> >>> better as we can use "PLAT2 " in MCFG table and not worry about the
>> >>> "PLAT" sub-string match causes the quirk to be applied
>> >>> unintentionally.
>> >
>> >> Note that platforms already shipped where OEM string has no padding will
>> >
>> > I'm confused by this statement. OEMID is defined as 6 bytes long and OEM
>> > Table ID as 8 bytes long in the ACPI specification. As far as I can
>> > tell, if your string isn't exactly that long, padding up to that length
>> > is required.
>> >
>> >> have change the firmware or add 0 padding to our quirk array IDs.
>> >
>> > The fixed 6 or 8 character string compare, as used v2 of this patchset,
>> > will be compatible with existing firmware as best I can tell. Adding
>> > padding to the quirk array IDs is exactly what I'm suggesting, although
>> > all the strings I've seen are space padded rather than null padded.
>> >
>>
>> I don't think any interpretation of the 6 or 8 byte wide OEM fields is
>> necessary to be able to match it against a list of known values as
>> used by the quirky platforms. We need an exact match against whatever
>> we know is in the table of an affected system, and whether a space
>> qualifies as padding or as a character is irrelevant.
>>
>> > Matches:
>> > {"APM   ", "XGENE   ", 1}
>> > {"CAVIUM", "THUNDERX", 1}
>> > {"HISI  ", "HISI-D02", 1}
>> > {"HISI  ", "HISI-D03", 1}
>> > {"QCOM  ", "QDF2432 ", 1}
>> >
>>
>> I would not mind listing these as
>>
>> { { 'A','P','M',' ',' ',' ',' '}, {'X','G','E','N','E',' ',' ',' '}, 1}
>> ...
>>
>> just to stress that we are not dealing with C strings (and to avoid
>> having to deal with the implicit NUL terminator).
>> That also means memcmp() with a fixed length is the most appropriate
>> to perform the comparison
>
> I still would go with memcmp but have the char arrays null terminated
> in addition. This first makes string handling easier, and fixes some
> unterminated %s printfs bugs in the code.
>
> Thus, I would prefer to go with:
>
> struct pci_cfg_fixup {
>         char oem_id[ACPI_OEM_ID_SIZE + 1];
>         char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
>         ...
>
> static struct pci_cfg_quirks mcfg_qurks[] __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, ...
>
> This is also no "pain in the eyes". :)
>
> If there are zero bytes in then just use \0, e.g.:
>
>  { "foo\0\0\0", "foobar\0\0", ... }
>
> For comparisation still use memcmp accordingly:
>
>  memcmp(..., ACPI_OEM_ID_SIZE);
>  memcmp(..., ACPI_OEM_TABLE_ID_SIZE);
>
> The following would be fixed too as strings are now null terminated:
>
>         pr_info("Handling %s %s r%d PCI MCFG quirks\n",
>                 f->oem_id, f->oem_table_id, f->oem_revision);
>

This looks like a clear improvement to me.

> Btw, use dev_info(&root->device->dev, ...) here for pr_info() and
> modify message text, e.g.:
>
>  acpi PNP0A08:04: Applying PCI MCFG quirks for CAVIUM THUNDERX rev: 1
>
> And, we should support some sort of MCFG_OEM_REVISION_ANY to move the
> rev handling optional to pci_cfg_fixup::init().
>

xxx_ANY implies 'wildcard', which we don't want in this code. The set
of quirky hardware we intend to support is known, and wildcard
matching makes it easier to circumvent our policy that from here on,
i.e., that all ACPI/arm64 supported hardware shall adhere to the spec.

-- 
Ard.

[RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks

[Robert Richter]

On 22.07.16 14:00:42, Ard Biesheuvel wrote:
> On 22 July 2016 at 13:38, Robert Richter <rric@kernel.org> wrote:

> > And, we should support some sort of MCFG_OEM_REVISION_ANY to move the
> > rev handling optional to pci_cfg_fixup::init().
> >
> 
> xxx_ANY implies 'wildcard', which we don't want in this code. The set
> of quirky hardware we intend to support is known, and wildcard
> matching makes it easier to circumvent our policy that from here on,
> i.e., that all ACPI/arm64 supported hardware shall adhere to the spec.

Fine with me, there shouln't be to many revs around to have a quirk
per rev.

-Robert

[RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks

[Mark Salter]

On Tue, 2016-06-28 at 09:54 +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 config access accordingly to quirk requirements.
> If no custom init call 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 | 88 ++++++++++++++++++++++++++++++++++++++++++
> ?drivers/pci/host/mcfg-quirks.h | 20 ++++++++++
> ?include/linux/pci-acpi.h???????|??2 +
> ?4 files changed, 111 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 5bc0af2..e3d55a0 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -30,3 +30,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..fb2b184
> --- /dev/null
> +++ b/drivers/pci/host/mcfg-quirks.c
> @@ -0,0 +1,88 @@
> +/*
> + * 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;
> +	char *oem_table_id;
> +	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_qurks[] __initconst = {
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?^^^^^^^^^^
mcfg_quirks

> +/*	{ 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)
> +{
> +	int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
> +	int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
> +
> +	return (!strncmp(f->oem_id, mcfg_header->oem_id, olen) &&
> +		!strncmp(f->oem_table_id, mcfg_header->oem_table_id, tlen) &&
> +		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_qurks;
> +	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;
> +
> +	/*
> +	?* 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_qurks); i++, f++) {
> +		if (resource_contains(&f->domain_range, &dom_res) &&
> +		????resource_contains(&f->bus_range, bus_res) &&
> +		????pci_mcfg_fixup_match(f, mcfg_header)) {
> +			pr_info("Handling %s %s r%d PCI MCFG quirks\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);
> +}
> 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);

[RFC PATCH v4 4/5] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Tomasz Nowicki]

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 addf00c..ef142b8 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -152,8 +152,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);
 	if (!ri->cfg) {
 		kfree(ri);
 		return NULL;
-- 
1.9.1

[RFC PATCH v4 5/5] PCI: thunder: Add ThunderX PEM MCFG quirk to the list

[Tomasz Nowicki]

Add ThunderX PEM specific infrastructure to support configuration
access for ACPI based PCI host controller. This involves:
1. New initialization call thunder_pem_cfg_init() to create configuration
space mapping with hardcoded range address
2. thunder_pem_init() ACPI extension to get hardcoded address for
PEM specific register range
3. New entry in common quirk array.

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 fb2b184..a4bb76a 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_qurks[] __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 91f6fc6..73ac537 100644
--- a/drivers/pci/host/pci-thunder-pem.c
+++ b/drivers/pci/host/pci-thunder-pem.c
@@ -18,9 +18,12 @@
 #include <linux/module.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;
 	}
 
@@ -362,5 +396,41 @@ static struct platform_driver thunder_pem_driver = {
 };
 module_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);
+	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;
+}
+#endif
+
 MODULE_DESCRIPTION("Thunder PEM PCIe host driver");
 MODULE_LICENSE("GPL v2");
-- 
1.9.1