From: Bjorn Helgaas <helgaas@kernel.org>
To: Tomasz Nowicki <tn@semihalf.com>
Cc: linux-pci@vger.kernel.org,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
Gabriele Paoloni <gabriele.paoloni@huawei.com>,
"Rafael J. Wysocki" <rafael@kernel.org>,
Duc Dang <dhdang@apm.com>, Sinan Kaya <okaya@codeaurora.org>,
Christopher Covington <cov@codeaurora.org>,
Dongdong Liu <liudongdong3@huawei.com>
Subject: Re: [PATCH v10 00/12] PCI: ARM64 ECAM quirks
Date: Tue, 6 Dec 2016 12:56:55 -0600 [thread overview]
Message-ID: <20161206185655.GA554@bhelgaas-glaptop.roam.corp.google.com> (raw)
In-Reply-To: <2bef38c3-be46-918e-8ab3-1eb39df23edc@semihalf.com>
On Mon, Dec 05, 2016 at 01:36:01PM +0100, Tomasz Nowicki wrote:
> On 02.12.2016 22:57, Bjorn Helgaas wrote:
> >On Fri, Dec 02, 2016 at 03:20:28PM +0100, Tomasz Nowicki wrote:
> >>dmesg from ThunderX pass2.0 (1 socket board) + small fix:
> I believe Robert meant:
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index d34d196..8ca8ca8 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -76,7 +76,7 @@ static struct mcfg_fixup mcfg_quirks[] = {
> HISI_QUAD_DOM("HIP07 ", 12, &hisi_pcie_ops),
>
> #define THUNDER_PEM_RES(addr, node) \
> - DEFINE_RES_MEM(addr + (node << 44), 0x39 * SZ_16M)
> + DEFINE_RES_MEM(addr + ((u64)node << 44), 0x39 * SZ_16M)
>
> Which means we can forget UL suffix for THUNDER_PEM_QUIRK macro:
> @@ -91,8 +91,8 @@ static struct mcfg_fixup mcfg_quirks[] = {
> { "CAVIUM", "THUNDERX", rev, 9 + (10 * node), MCFG_BUS_ANY,
> \
> &thunder_pem_ecam_ops, THUNDER_PEM_RES(0x89808f000000UL, node) }
> /* SoC pass2.x */
> - THUNDER_PEM_QUIRK(1, 0UL),
> - THUNDER_PEM_QUIRK(1, 1UL),
> + THUNDER_PEM_QUIRK(1, 0),
> + THUNDER_PEM_QUIRK(1, 1),
>
> Also, my apologies, I should have fixed parentheses in this macro
> from the very beginning. For you convenience below incremental patch
> where I fixed both issues:
>
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index d34d196..cf6c321 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -76,23 +76,23 @@ static struct mcfg_fixup mcfg_quirks[] = {
> HISI_QUAD_DOM("HIP07 ", 12, &hisi_pcie_ops),
>
> #define THUNDER_PEM_RES(addr, node) \
> - DEFINE_RES_MEM(addr + (node << 44), 0x39 * SZ_16M)
> + DEFINE_RES_MEM((addr) + ((u64)(node) << 44), 0x39 * SZ_16M)
> #define THUNDER_PEM_QUIRK(rev, node) \
> - { "CAVIUM", "THUNDERX", rev, 4 + (10 * node), MCFG_BUS_ANY,
> \
> - &thunder_pem_ecam_ops, THUNDER_PEM_RES(0x88001f000000UL,
> node) }, \
> - { "CAVIUM", "THUNDERX", rev, 5 + (10 * node), MCFG_BUS_ANY,
> \
> - &thunder_pem_ecam_ops, THUNDER_PEM_RES(0x884057000000UL,
> node) }, \
> - { "CAVIUM", "THUNDERX", rev, 6 + (10 * node), MCFG_BUS_ANY,
> \
> - &thunder_pem_ecam_ops, THUNDER_PEM_RES(0x88808f000000UL,
> node) }, \
> - { "CAVIUM", "THUNDERX", rev, 7 + (10 * node), MCFG_BUS_ANY,
> \
> - &thunder_pem_ecam_ops, THUNDER_PEM_RES(0x89001f000000UL,
> node) }, \
> - { "CAVIUM", "THUNDERX", rev, 8 + (10 * node), MCFG_BUS_ANY,
> \
> - &thunder_pem_ecam_ops, THUNDER_PEM_RES(0x894057000000UL,
> node) }, \
> - { "CAVIUM", "THUNDERX", rev, 9 + (10 * node), MCFG_BUS_ANY,
> \
> - &thunder_pem_ecam_ops, THUNDER_PEM_RES(0x89808f000000UL, node) }
> + { "CAVIUM", "THUNDERX", (rev), 4 + (10 * (node)),
> MCFG_BUS_ANY, \
> + &thunder_pem_ecam_ops, THUNDER_PEM_RES(0x88001f000000UL,
> (node)) }, \
> + { "CAVIUM", "THUNDERX", (rev), 5 + (10 * (node)),
> MCFG_BUS_ANY, \
> + &thunder_pem_ecam_ops, THUNDER_PEM_RES(0x884057000000UL,
> (node)) }, \
> + { "CAVIUM", "THUNDERX", (rev), 6 + (10 * (node)),
> MCFG_BUS_ANY, \
> + &thunder_pem_ecam_ops, THUNDER_PEM_RES(0x88808f000000UL,
> (node)) }, \
> + { "CAVIUM", "THUNDERX", (rev), 7 + (10 * (node)),
> MCFG_BUS_ANY, \
> + &thunder_pem_ecam_ops, THUNDER_PEM_RES(0x89001f000000UL,
> (node)) }, \
> + { "CAVIUM", "THUNDERX", (rev), 8 + (10 * (node)),
> MCFG_BUS_ANY, \
> + &thunder_pem_ecam_ops, THUNDER_PEM_RES(0x894057000000UL,
> (node)) }, \
> + { "CAVIUM", "THUNDERX", (rev), 9 + (10 * (node)),
> MCFG_BUS_ANY, \
> + &thunder_pem_ecam_ops, THUNDER_PEM_RES(0x89808f000000UL, (node)) }
> /* SoC pass2.x */
> - THUNDER_PEM_QUIRK(1, 0UL),
> - THUNDER_PEM_QUIRK(1, 1UL),
> + THUNDER_PEM_QUIRK(1, 0),
> + THUNDER_PEM_QUIRK(1, 1),
>
> #define THUNDER_ECAM_QUIRK(rev, seg) \
> { "CAVIUM", "THUNDERX", rev, seg, MCFG_BUS_ANY, \
> diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
> index 00eb8eb..643c9e7 100644
> --- a/include/linux/pci-ecam.h
> +++ b/include/linux/pci-ecam.h
> @@ -62,8 +62,9 @@ extern struct pci_ecam_ops pci_generic_ecam_ops;
> #if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
> extern struct pci_ecam_ops pci_32b_ops; /* 32-bit
> accesses only */
> extern struct pci_ecam_ops hisi_pcie_ops; /* HiSilicon */
> -extern struct pci_ecam_ops thunder_pem_ecam_ops; /* Cavium ThunderX
> 1.x & 2.x */
> -extern struct pci_ecam_ops pci_thunder_ecam_ops; /* Cavium ThunderX 1.x */
> +extern struct pci_ecam_ops thunder_pem_ecam_ops; /* Cavium ThunderX
> pass1.x &
> + pass2.x */
> +extern struct pci_ecam_ops pci_thunder_ecam_ops; /* Cavium ThunderX
> pass1.x */
> #endif
I applied the above by hand, thanks!
> >>ACPI: MCFG table detected, 4 entries
> >>ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-1f])
> >>acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM
> >>Segments MSI]
> >>acpi PNP0A08:00: _OSC: platform does not support [PCIeHotplug PME AER]
> >>acpi PNP0A08:00: _OSC: OS now controls [PCIeCapability]
> >>acpi PNP0A08:00: ECAM area [mem 0x848000000000-0x848001ffffff]
> >>reserved by THRX0001:00
> >>acpi PNP0A08:00: ECAM at [mem 0x848000000000-0x848001ffffff] for [bus 00-1f]
> >
> >I guess we don't need MCFG quirks for these bridges, since I don't see
> >the "MCFG quirk" message?
>
> Yes, this is ThunderX 1-socket pass2.x so quirks are needed for 4-9
> host bridges only. Below summary of where/how quirks should be
> applied:
>
> ThunderX pass2.x
> NUMA node -> segments -> ECAM compliant note
> 0 -> 0-3 -> ECAM compliant
> 0 -> 4-9 -> ECAM quirks (thunder_pem_ecam_ops)
> 1 (optionally) -> 10-13 -> ECAM compliant
> 1 (optionally) -> 14-19 -> ECAM quirks (thunder_pem_ecam_ops)
>
> ThunderX pass1.x
> NUMA node -> segments -> ECAM compliant note
> 0 -> 0-3 -> ECAM quirks (pci_thunder_ecam_ops)
> 0 -> 4-9 -> ECAM quirks (thunder_pem_ecam_ops)
> 1 (optionally) -> 10-13 -> ECAM quirks (pci_thunder_ecam_ops)
> 1 (optionally) -> 14-19 -> ECAM quirks (thunder_pem_ecam_ops)
I included the above in the appropriate changelogs.
> >>system 00:00: [mem 0x848000000000-0x848001ffffff] could not be reserved
> >>system 00:01: [mem 0x849000000000-0x849001ffffff] could not be reserved
> >>system 00:02: [mem 0x84a000000000-0x84a001ffffff] could not be reserved
> >>system 00:03: [mem 0x84b000000000-0x84b001ffffff] could not be reserved
> >>system 00:04: [mem 0x87e0c0000000-0x87e0c0ffffff] could not be reserved
> >>system 00:04: [mem 0x88001f000000-0x880057ffffff] could not be reserved
> >>system 00:05: [mem 0x87e0c2000000-0x87e0c2ffffff] could not be reserved
> >>system 00:05: [mem 0x88808f000000-0x8880c7ffffff] could not be reserved
> >
> >Most of these match ECAM spaces, which is good. 00:04 and 00:05 each
> >have one ECAM space and one "other" space, which might be PEMx host
> >bridge registers? That all seems good.
>
> Yes, "other" space is PEMx host bridge registers.
>
> >But I assume the other bridges (PCIx) also have register space in
> >addition to ECAM, and that should be reported also.
>
> 00:00, 00:01, 00:02, 00:03 are ECAM compliant so for those we need
> ECAM region only.
Even if they're ECAM compliant, I'm surprised that the bridges have no
other register space, e.g., for programming the range of bus numbers
below the bridge, root complex error reporting, etc. This would all
be device-specific, of course, so in the ACPI model it would be used
by firmware only (either firmware init or by ASL).
> >>root@ubuntu:~# cat /proc/iomem
> >>...
> >>838000000000-841fffffffff : PCI Bus 0000:00
> >> 838000000000-8380003fffff : 0000:03:00.0
> >
> >Something's missing here: we should have a clue about how we got from
> >bus 00 to bus 03. From your dmesg, 0000:00:15.0 is a bridge from bus
> >00 to bus 03, and its windows should appear here. I'd expect
> >something like:
> >
> > 838000000000-841fffffffff : PCI Bus 0000:00
> > 838000000000-838fffffffff : PCI Bus 0000:03 <- 00:15.0 window
> > 838000000000-8380003fffff : 0000:03:00.0
> >
> >This window should be inserted by generic code, so I don't know how
> >this could be broken. Your dmesg should also have something like
> >this:
> >
> > pci 0000:00:15.0: PCI bridge to [bus 03]
> > pci 0000:00:15.0: bridge window [mem 0x838000000000-0x838fffffffff]
> >
> >I don't see that, maybe because this is actually a console log
> >collected without "ignore_loglevel"? But that obviously doesn't
> >affect /proc/iomem, so I'm still puzzled about that.
>
> Enhanced Allocation is used for devices but not for bridges which
> have invalid BARs in standard configuration header. Thus devices
> request resources from first valid parent bridge (host bridge in
> this case).
Hmm, OK. I can tell EA is going to be a pain. We might need to
improve our logging somehow to make this more visible.
For my own edification, does this mean the bridge itself has magic EA
functionality? I was assuming EA would be used for non-configurable
endpoints. If the bridges leading to those endpoints have extra
apertures not described by the standard bridge window registers, that
makes it much more complicated to understand the topology.
> >>87e024000000-87e024000fff : ARMH0011:00
> >> 87e024000000-87e024000fff : ARMH0011:00
> >
> >This is interesting. This must be a driver reserving these areas?
> >Normally a driver would use the driver name, not the device name.
> >
> >Ideally, I think the core should reserve the region with the device
> >name, and the driver would request it using the driver name, like
> >this:
> >
> > 843000000000-84303fffffff : 0002:01:00.0 <-- PCI core reservation
> > 843000000000-84303fffffff : thunder-nic <-- driver request
> >
> >The ACPI core doesn't actually do the reservation, so I assume the
> >ARMH0011:00 stuff is from the driver, and it's curious that it has the
> >same region twice.
> >
> >>87e026000000-87e0bfffffff : PCI Bus 0000:00
> >> 87e027000000-87e0277fffff : CAVA02A:00
> >
> >This is interesting, too. CAVA02A:00 looks like an ACPI device, but
> >apparently it consumes some of the space that we think is routed to
> >PCI bus 0000:00. I think this happens on some x86 boxes, too, but it
> >is somewhat confusing.
>
> I will take a look.
>
> >
> >Based on this, I don't see any problems with the ThunderX quirks.
> >I'd like to understand what's going on with the PCI-to-PCI bridge
> >windows in /proc/iomem, but I doubt that's related to your quirks.
>
> Yes it is not related to quirks.
>
> However, CAVA02A:00 and ARMH0011 things should be investigated.
Thanks! This piece is more a curiosity thing on my part.
ARMH0011 looks like it's claimed by arm_sbsa_uart_platform_driver.
This path:
sbsa_uart_probe
pl011_setup_port
devm_ioremap_resource
devm_request_mem_region
explains one of the lines, still curious about the other.
Bjorn
prev parent reply other threads:[~2016-12-06 18:57 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-01 8:29 [PATCH v10 00/12] PCI: ARM64 ECAM quirks Bjorn Helgaas
2016-12-01 8:29 ` [PATCH v10 01/12] ACPI: Add acpi_resource_consumer() to find device that claims a resource Bjorn Helgaas
2016-12-01 13:17 ` Rafael J. Wysocki
2016-12-01 16:35 ` Bjorn Helgaas
2016-12-01 8:29 ` [PATCH v10 02/12] PCI: Search ACPI namespace to ensure ECAM space is reserved Bjorn Helgaas
2016-12-01 14:17 ` Lorenzo Pieralisi
2016-12-01 18:05 ` Bjorn Helgaas
2016-12-01 8:29 ` [PATCH v10 03/12] x86/PCI: Use acpi_resource_consumer() to search ACPI namespace for MMCFG Bjorn Helgaas
2016-12-01 8:29 ` [PATCH v10 04/12] arm64: PCI: Manage controller-specific data on per-controller basis Bjorn Helgaas
2016-12-01 8:30 ` [PATCH v10 05/12] PCI/ACPI: Extend pci_mcfg_lookup() to return ECAM config accessors Bjorn Helgaas
2016-12-01 8:30 ` [PATCH v10 06/12] PCI/ACPI: Check for platform-specific MCFG quirks Bjorn Helgaas
2016-12-01 8:30 ` [PATCH v10 07/12] PCI/ACPI: Provide acpi_get_rc_resources() for ARM64 platform Bjorn Helgaas
2016-12-01 8:30 ` [PATCH v10 08/12] PCI: Add MCFG quirks for Qualcomm QDF2432 host controller Bjorn Helgaas
2016-12-01 8:30 ` [PATCH v10 09/12] PCI: Add MCFG quirks for HiSilicon Hip05/06/07 host controllers Bjorn Helgaas
2016-12-01 8:30 ` [PATCH v10 10/12] PCI: thunder-pem: Factor out resource lookup Bjorn Helgaas
2016-12-01 8:30 ` [PATCH v10 11/12] PCI: Add MCFG quirks for Cavium ThunderX pass2.x host controller Bjorn Helgaas
2016-12-02 17:35 ` Bjorn Helgaas
2016-12-05 8:32 ` Tomasz Nowicki
2016-12-01 8:31 ` [PATCH v10 12/12] PCI: Add MCFG quirks for Cavium ThunderX pass1.x " Bjorn Helgaas
2016-12-01 19:02 ` Tomasz Nowicki
2016-12-01 20:24 ` Bjorn Helgaas
2016-12-01 13:02 ` [PATCH v10 00/12] PCI: ARM64 ECAM quirks Dongdong Liu
2016-12-01 14:04 ` Dongdong Liu
2016-12-01 16:31 ` Bjorn Helgaas
2016-12-02 3:46 ` Dongdong Liu
2016-12-02 8:15 ` Hanjun Guo
2016-12-02 21:22 ` Bjorn Helgaas
2016-12-01 18:01 ` Bjorn Helgaas
2016-12-02 11:42 ` Dongdong Liu
2016-12-01 21:53 ` Gabriele Paoloni
2016-12-02 14:20 ` Tomasz Nowicki
2016-12-02 21:57 ` Bjorn Helgaas
2016-12-05 12:36 ` Tomasz Nowicki
2016-12-06 18:56 ` Bjorn Helgaas [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20161206185655.GA554@bhelgaas-glaptop.roam.corp.google.com \
--to=helgaas@kernel.org \
--cc=cov@codeaurora.org \
--cc=dhdang@apm.com \
--cc=gabriele.paoloni@huawei.com \
--cc=linux-pci@vger.kernel.org \
--cc=liudongdong3@huawei.com \
--cc=lorenzo.pieralisi@arm.com \
--cc=okaya@codeaurora.org \
--cc=rafael@kernel.org \
--cc=tn@semihalf.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).