On Thu, Dec 01, 2016 at 09:49:51AM +0100, Tomasz Nowicki wrote:

Hi Bjorn,

On 01.12.2016 01:28, Bjorn Helgaas wrote:

quoted

Hi Tomasz,

On Tue, Nov 15, 2016 at 10:14:57AM +0100, Tomasz Nowicki wrote:

quoted

ThunderX PCIe controller to off-chip devices (so-called PEM) is not fully
compliant with ECAM standard. It uses non-standard configuration space
accessors (see pci_thunder_pem_ops) and custom configuration space granulation
(see bus_shift = 24). In order to access configuration space and
probe PEM as ACPI based PCI host controller we need to add MCFG quirk
infrastructure. This involves:
1. thunder_pem_init() ACPI extension so that we can probe PEM-specific
  register ranges analogously to DT
2. Export PEM pci_thunder_pem_ops structure so it is visible to MCFG quirk
  code.
3. New quirk entries for each PEM segment. Each contains platform IDs,
  mentioned pci_thunder_pem_ops and CFG resources.

Quirk is considered for ThunderX silicon pass2.x only which is identified
via MCFG revision 1.

Signed-off-by: Tomasz Nowicki <redacted>
---
drivers/acpi/pci_mcfg.c            |  20 +++++++
drivers/pci/host/pci-thunder-pem.c | 107 ++++++++++++++++++++++++++++++++-----
include/linux/pci-ecam.h           |   4 ++
3 files changed, 117 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index ac21db3..e4e2b9b 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c

@@ -57,6 +57,26 @@ static struct mcfg_fixup mcfg_quirks[] = {

	{ "QCOM  ", "QDF2432 ", 1, 5, MCFG_BUS_ANY, &pci_32b_ops },
	{ "QCOM  ", "QDF2432 ", 1, 6, MCFG_BUS_ANY, &pci_32b_ops },
	{ "QCOM  ", "QDF2432 ", 1, 7, MCFG_BUS_ANY, &pci_32b_ops },
+#ifdef CONFIG_PCI_HOST_THUNDER_PEM
+#define THUNDER_MCFG_RES(addr, node) \
+	DEFINE_RES_MEM(addr + (node << 44), 0x39 * SZ_16M)
+#define THUNDER_MCFG_QUIRK(rev, node) \
+	{ "CAVIUM", "THUNDERX", rev, 4 + (10 * node), MCFG_BUS_ANY, \
+	  &pci_thunder_pem_ops, THUNDER_MCFG_RES(0x88001f000000UL, node) }, \
+	{ "CAVIUM", "THUNDERX", rev, 5 + (10 * node), MCFG_BUS_ANY, \
+	  &pci_thunder_pem_ops, THUNDER_MCFG_RES(0x884057000000UL, node) }, \
+	{ "CAVIUM", "THUNDERX", rev, 6 + (10 * node), MCFG_BUS_ANY, \
+	  &pci_thunder_pem_ops, THUNDER_MCFG_RES(0x88808f000000UL, node) }, \
+	{ "CAVIUM", "THUNDERX", rev, 7 + (10 * node), MCFG_BUS_ANY, \
+	  &pci_thunder_pem_ops, THUNDER_MCFG_RES(0x89001f000000UL, node) }, \
+	{ "CAVIUM", "THUNDERX", rev, 8 + (10 * node), MCFG_BUS_ANY, \
+	  &pci_thunder_pem_ops, THUNDER_MCFG_RES(0x894057000000UL, node) }, \
+	{ "CAVIUM", "THUNDERX", rev, 9 + (10 * node), MCFG_BUS_ANY, \
+	  &pci_thunder_pem_ops, THUNDER_MCFG_RES(0x89808f000000UL, node) }
+	/* SoC pass2.x */
+	THUNDER_MCFG_QUIRK(1, 0UL),
+	THUNDER_MCFG_QUIRK(1, 1UL),
+#endif

I want all these quirks to work without having to enable
device-specific config options like CONFIG_PCI_HOST_THUNDER_PEM.

I tweaked the preceding MCFG quirk support to wrap it in
CONFIG_PCI_QUIRKS.  I also tweaked the qualcomm and hisi patches to
move the meat of them to pci/quirks.c.  My work-in-progress is on
pci/ecam, but I can't easily build for arm64, so there are likely some
issues to be resolved.

Please see compilation fix patch in [1] for your series.

quoted

I'm hoping to end up with something like this:
https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/ecam&id=51ad4df79a9b7f2a66b346a46b21a785a2937469

The problem with ThunderX is that the config accessors are much bigger
and I don't really want to duplicate them in both pci/quirks.c and
pci-thunder-pem.c.

Actually, that raises a question for qualcomm and hisi: in the DT
model, we use non-ECAM config accessors in the driver, but in the ACPI
model, we use ECAM accessors.  It seems like the accessors should be
the same regardless of whether we discover the bridge via DT or ACPI.

Anyway, it's almost like we want to build pci-thunder-pem.o if
CONFIG_PCI_HOST_THUNDER_PEM || (CONFIG_PCI_QUIRKS && CONFIG_ARM64).
I don't know how to express that nicely.

I was trying to avoid adding an ecam-quirks.c, but maybe we need to
add it and collect all the different accessors there and add #ifdefs
inside.

Sorry, this is only half-baked, but I just wanted to throw this out in
case you have any ideas.

I agree that pci-thunder-pem.c and pci-thunder-ecam.c are too big to
be duplicated. The same for new ecam-quirks.c container. So treating
this as a necessary evil how about:

I discarded my original half-baked ecam-quirks.c idea and instead
tried changing the Makefile and adding some ifdefs in the individual
drivers.  It's not too pretty, but maybe better than ecam-quirks.c?

 config PCI_HOST_THUNDER_PEM
 	bool "Cavium Thunder PCIe controller to off-chip devices"
-	depends on OF && ARM64
+	depends on ARM64
+	depends on OF || (ACPI && PCI_QUIRKS)
 	select PCI_HOST_COMMON

Moreover, IMO we should select PCI_QUIRKS for ARM64 && ACPI by
default. Then it becomes:
 config PCI_HOST_THUNDER_PEM
 	bool "Cavium Thunder PCIe controller to off-chip devices"
-	depends on OF && ARM64
+	depends on ARM64
+	depends on OF || ACPI
 	select PCI_HOST_COMMON

Thanks, I think you're right about needing Kconfig changes here.

I incorporated the first one (with "(ACPI && PCI_QUIRKS)") because we
shouldn't offer this Kconfig choice when PCI_QUIRKS is not set.

If we only depend on ACPI here and we decide that ARM64 should *not*
automatically select PCI_QUIRKS, we'll offer this choice even when
PCI_QUIRKS is not set, and that seems wrong.

Making it explicit here removes the non-obvious connection with the
PCI_QUIRKS selection strateg.

Bjorn