From: bhelgaas@google•com (Bjorn Helgaas)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH v2 1/2] PCI: generic: remove dependency on hw_pci
Date: Wed, 13 May 2015 10:11:45 -0500 [thread overview]
Message-ID: <20150513151145.GB27374@google.com> (raw)
In-Reply-To: <55536833.2090002@amd.com>
On Wed, May 13, 2015 at 10:05:23AM -0500, Suravee Suthikulanit wrote:
> On 5/13/2015 8:54 AM, Bjorn Helgaas wrote:
> >Hi Suravee,
> >
> >On Wed, May 13, 2015 at 07:47:42AM -0500, Suravee Suthikulanit wrote:
> >>On 5/6/2015 10:32 PM, Suravee Suthikulanit wrote:
> >>>I tested this patch series on the AMD Seattle w/o PCI_PROBE_ONLY mode
> >>>and that works fine. However, w/ PCI_PROBE_ONLY, I also run into the
> >>>resource not claimed issue (no surprise here).
> >>>
> >>>So, I tried porting the pcibios_claim_one_bus() from
> >>>arch/alpha/kernel/pci.c as Lorenzo suggested, plus the a small change in
> >>>pci_claim_resource(), and it seems to work w/ PCI_PROBE_ONLY. (Please
> >>>see example patch below.)
> >>>
> >>>The additional while loop is needed in the pci_claim_resource() since I
> >>>need to reference back to the resource in the root bus, which are
> >>>defined from the DT node. Does this sounds like a reasonable approach?
> >>>
> >>>diff --git a/drivers/pci/host/pci-host-generic.c
> >>>b/drivers/pci/host/pci-host-generic.c
> >>>index e9cc559..0dfa23d 100644
> >>>--- a/drivers/pci/host/pci-host-generic.c
> >>>+++ b/drivers/pci/host/pci-host-generic.c
> >>>@@ -261,7 +261,10 @@ static int gen_pci_probe(struct platform_device *pdev)
> >>> if (!pci_has_flag(PCI_PROBE_ONLY)) {
> >>> pci_bus_size_bridges(bus);
> >>> pci_bus_assign_resources(bus);
> >>>+ } else {
> >>>+ pci_claim_one_bus(bus);
> >>> }
> >>>+
> >>> pci_bus_add_devices(bus);
> >>>
> >>> /* Configure PCI Express settings */
> >>>diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> >>>index 232f925..d4b43b3 100644
> >>>--- a/drivers/pci/setup-res.c
> >>>+++ b/drivers/pci/setup-res.c
> >>>@@ -109,6 +109,7 @@ int pci_claim_resource(struct pci_dev *dev, int
> >>>resource)
> >>> {
> >>> struct resource *res = &dev->resource[resource];
> >>> struct resource *root, *conflict;
> >>>+ struct pci_dev *pdev = dev;
> >>>
> >>> if (res->flags & IORESOURCE_UNSET) {
> >>> dev_info(&dev->dev, "can't claim BAR %d %pR: no address
> >>>assigned\n",
> >>>@@ -116,7 +117,18 @@ int pci_claim_resource(struct pci_dev *dev, int
> >>>resource)
> >>> return -EINVAL;
> >>> }
> >>>
> >>>- root = pci_find_parent_resource(dev, res);
> >>>+ while (pdev) {
> >>>+ root = pci_find_parent_resource(pdev, res);
> >>>+ if (root)
> >>>+ break;
> >>>+
> >>>+ if (pci_has_flag(PCI_PROBE_ONLY) &&
> >>>+ !pci_is_root_bus(pdev->bus))
> >>>+ pdev = pdev->bus->self;
> >>>+ else
> >>>+ break;
> >>>+ }
> >>>+
> >>> if (!root) {
> >>> dev_info(&dev->dev, "can't claim BAR %d %pR: no compatible
> >>>bridge window\n",
> >>> resource, res);
> >>
> >>
> >>[From Bjorn]
> >>I don't understand this new loop. Apparently you have a device BAR, and
> >>the upstream bridge doesn't have a window that contains the BAR? That
> >>sounds like a problem with the upstream bridge resources.
> >>
> >>Do you have an example that would make this more concrete, e.g., a host
> >>bridge, P2P bridge(s), and endpoint with their resources?
> >>
> >>[Suravee]
> >>Here is the information you were asking for. This information is
> >>setup from the FW. In the PCI bridge (00:02.1), I see the
> >>prefetchable memory behind bridge is already setup.
> >
> >Here's a summary:
> >
> > 00:02.1: PCI bridge to [bus 01]
> > 00:02.1: [io window disabled]
> > 00:02.1: [mem window disabled]
> > 00:02.1: bridge window [mem 0x7f_ffe0_0000-0x7f_ffff_ffff 64bit pref]
> > 01:00.0: BAR 0: [mem 0x7f_ffe8_0000-0x... 64bit pref]
> > 01:00.0: BAR 2: [io 0x0020-0x3f]
> > 01:00.0: BAR 4: [mem 0x7f_fff0_4000-0x... 64bit pref]
> >
> >So I guess the new loop would be for the I/O resource because the
> >00:02.1 I/O window is disabled? This definitely seems like a problem --
> >we should enable that I/O window so we can claim the 01:00.0 I/O
> >resource with the 00:02.1 I/O window as the parent. We don't want the
> >parent to be the host bridge window. In fact, unless we enable that
> >I/O window, the 01:00.0 I/O BAR won't even work!
> >
> >It may be that the driver for 01:00.0 doesn't need the I/O BAR. We
> >can leave the bridge I/O window disabled and let pci_claim_resource()
> >fail, which means we'll treat the 01:00.0 I/O BAR as unassigned.
> >That's all fine; we'll never turn on PCI_COMMAND_IO, and as long as
> >the driver doesn't request I/O space, everything should just work.
> >Note that the driver would have to use pci_enable_device_mem() to
> >tell us that it doesn't need I/O space.
> >
>
> I see your point on the I/O window. Let me double check the FW why
> this is getting setup this way. My guess is that the I/O windows are
> not used by ARM64 systems, therefore the FW disabled it at the
> bridge.
>
> However, the loop is mainly to recursively search for parent
> resource for the 64bit pref resource of device 1:00.0 when calling
> pci_claim_one_bus() on PROBE_ONLY. It doesn't seem to find
> compatible bridge windows in the parent bridge (0:2.1), and I was
> not sure if that is where the device is supposed to be claiming
> from.
An endpoint should claim resources from its immediate upstream bridge.
There's no point in claiming a resource from the top-level bridge if a
bridge in the middle doesn't forward that resource down the the endpoint.
Bjorn
next prev parent reply other threads:[~2015-05-13 15:11 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-05 2:02 [PATCH v2 1/2] PCI: generic: remove dependency on hw_pci Jayachandran C
2015-05-05 2:02 ` [PATCH v2 2/2] PCI: generic: add arm64 support Jayachandran C
2015-05-05 15:53 ` [PATCH v2 1/2] PCI: generic: remove dependency on hw_pci Will Deacon
2015-05-05 15:58 ` Arnd Bergmann
2015-05-05 16:03 ` Lorenzo Pieralisi
2015-05-06 14:18 ` Lorenzo Pieralisi
2015-05-06 15:18 ` Bjorn Helgaas
2015-05-07 3:32 ` Suravee Suthikulanit
2015-05-12 13:34 ` Bjorn Helgaas
2015-05-12 16:34 ` Lorenzo Pieralisi
2015-05-12 19:20 ` Bjorn Helgaas
2015-05-13 12:47 ` Suravee Suthikulanit
2015-05-13 13:54 ` Bjorn Helgaas
2015-05-13 15:05 ` Suravee Suthikulanit
2015-05-13 15:11 ` Bjorn Helgaas [this message]
2015-05-12 0:07 ` Jayachandran C.
2015-05-19 23:09 ` Bjorn Helgaas
2015-05-20 17:29 ` Will Deacon
2015-05-20 20:46 ` Bjorn Helgaas
2015-05-21 6:37 ` Jayachandran C.
2015-05-26 9:59 ` Will Deacon
2015-05-26 10:38 ` Arnd Bergmann
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=20150513151145.GB27374@google.com \
--to=bhelgaas@google$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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