On Thu, Dec 01, 2016 at 09:49:51AM +0100, Tomasz Nowicki wrote:
> Hi Bjorn,
>
> On 01.12.2016 01:28, Bjorn Helgaas wrote:
> >Hi Tomasz,
> >
> >On Tue, Nov 15, 2016 at 10:14:57AM +0100, Tomasz Nowicki wrote:
> >>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 <tn@xxxxxxxxxxxx>
> >>---
> >> 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.
>
> >
> >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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
