Message ID | 554ADCE0.8020603@amd.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Wed, May 06, 2015 at 10:32:48PM -0500, 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; > + } 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? > + > if (!root) { > dev_info(&dev->dev, "can't claim BAR %d %pR: no compatible bridge > window\n", > resource, res); > @@ -136,6 +148,36 @@ int pci_claim_resource(struct pci_dev *dev, int > resource) > } > EXPORT_SYMBOL(pci_claim_resource); > > +void pci_claim_one_bus(struct pci_bus *b) > +{ > + struct pci_dev *pdev; > + struct pci_bus *child_bus; > + > + list_for_each_entry(pdev, &b->devices, bus_list) { > + int i; > + > + for (i = 0; i < PCI_NUM_RESOURCES; i++) { > + struct resource *r = &pdev->resource[i]; > + > + if (r->parent || !r->start || !r->flags) > + continue; > + > + if (pci_has_flag(PCI_PROBE_ONLY) || > + (r->flags & IORESOURCE_PCI_FIXED)) { > + if (pci_claim_resource(pdev, i) == 0) > + continue; > + > + pci_claim_bridge_resource(pdev, i); > + } > + } > + } > + > + list_for_each_entry(child_bus, &b->children, node) { > + pci_claim_one_bus(child_bus); > + } > +} > +EXPORT_SYMBOL(pci_claim_one_bus); I'm not a fan of pci_claim_one_bus(), on the philosophical grounds that claiming resources is a per-device thing, and I don't want to encourage people to do it on a per-bus level. I'd rather claim them somewhere in the pci_device_add() path, as s390 does in pcibios_add_device(). In fact, I'd *like* to do it even earlier, when we read each BAR, so we could identify invalid or unassigned BARs immediately. > + > void pci_disable_bridge_window(struct pci_dev *dev) > { > dev_info(&dev->dev, "disabling bridge mem windows\n"); > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 353db8d..b59ad4b 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1085,6 +1085,7 @@ void > pci_assign_unassigned_bridge_resources(struct pci_dev *bridge); > void pci_assign_unassigned_bus_resources(struct pci_bus *bus); > void pci_assign_unassigned_root_bus_resources(struct pci_bus *bus); > void pdev_enable_device(struct pci_dev *); > +void pci_claim_one_bus(struct pci_bus *b); > int pci_enable_resources(struct pci_dev *, int mask); > void pci_fixup_irqs(u8 (*)(struct pci_dev *, u8 *), > int (*)(const struct pci_dev *, u8, u8)); > -------- END PATCH ---- > > Thanks, > > Suravee >
On Tue, May 12, 2015 at 02:34:31PM +0100, Bjorn Helgaas wrote: [...] > > +void pci_claim_one_bus(struct pci_bus *b) > > +{ > > + struct pci_dev *pdev; > > + struct pci_bus *child_bus; > > + > > + list_for_each_entry(pdev, &b->devices, bus_list) { > > + int i; > > + > > + for (i = 0; i < PCI_NUM_RESOURCES; i++) { > > + struct resource *r = &pdev->resource[i]; > > + > > + if (r->parent || !r->start || !r->flags) > > + continue; > > + > > + if (pci_has_flag(PCI_PROBE_ONLY) || > > + (r->flags & IORESOURCE_PCI_FIXED)) { > > + if (pci_claim_resource(pdev, i) == 0) > > + continue; > > + > > + pci_claim_bridge_resource(pdev, i); > > + } > > + } > > + } > > + > > + list_for_each_entry(child_bus, &b->children, node) { > > + pci_claim_one_bus(child_bus); > > + } > > +} > > +EXPORT_SYMBOL(pci_claim_one_bus); > > I'm not a fan of pci_claim_one_bus(), on the philosophical grounds that > claiming resources is a per-device thing, and I don't want to encourage > people to do it on a per-bus level. > > I'd rather claim them somewhere in the pci_device_add() path, as s390 does > in pcibios_add_device(). In fact, I'd *like* to do it even earlier, when > we read each BAR, so we could identify invalid or unassigned BARs > immediately. You mean claiming the resources in __pci_read_base (and unset the resource if claiming it fails ?) regardless of PCI_PROBE_ONLY ? I will give it a go, I fear it might trigger regressions on other archs though. We could claim the resources in pcibios_add_device on arm64, but this means arm code should be patched too since I am not happy at all to let arm and arm64 diverge even more. Lorenzo
On Tue, May 12, 2015 at 11:34 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Tue, May 12, 2015 at 02:34:31PM +0100, Bjorn Helgaas wrote: > > [...] > >> > +void pci_claim_one_bus(struct pci_bus *b) >> > +{ >> > + struct pci_dev *pdev; >> > + struct pci_bus *child_bus; >> > + >> > + list_for_each_entry(pdev, &b->devices, bus_list) { >> > + int i; >> > + >> > + for (i = 0; i < PCI_NUM_RESOURCES; i++) { >> > + struct resource *r = &pdev->resource[i]; >> > + >> > + if (r->parent || !r->start || !r->flags) >> > + continue; >> > + >> > + if (pci_has_flag(PCI_PROBE_ONLY) || >> > + (r->flags & IORESOURCE_PCI_FIXED)) { >> > + if (pci_claim_resource(pdev, i) == 0) >> > + continue; >> > + >> > + pci_claim_bridge_resource(pdev, i); >> > + } >> > + } >> > + } >> > + >> > + list_for_each_entry(child_bus, &b->children, node) { >> > + pci_claim_one_bus(child_bus); >> > + } >> > +} >> > +EXPORT_SYMBOL(pci_claim_one_bus); >> >> I'm not a fan of pci_claim_one_bus(), on the philosophical grounds that >> claiming resources is a per-device thing, and I don't want to encourage >> people to do it on a per-bus level. >> >> I'd rather claim them somewhere in the pci_device_add() path, as s390 does >> in pcibios_add_device(). In fact, I'd *like* to do it even earlier, when >> we read each BAR, so we could identify invalid or unassigned BARs >> immediately. > > You mean claiming the resources in __pci_read_base (and unset the resource > if claiming it fails ?) regardless of PCI_PROBE_ONLY ? That's what I'm thinking, but only in the long term. I doubt we're ready to go that far yet. I was thinking more along the lines of just getting it uniformly into the core first, maybe even incrementally, and only after that moving it around inside the core. I don't think it would work right now because I think we read device BARs before we read the apertures of the upstream bridge. > We could claim the resources in pcibios_add_device on arm64, but this > means arm code should be patched too since I am not happy at all to let > arm and arm64 diverge even more. I agree, it'd be nice to have arm and arm64 be similar, but from my point of view, they wouldn't have to be changed at the same time. To me, they just look like two different arches. Bjorn
Hi Bjorn, Somehow, I didn't get your reply email from the ML. So, I've captured your questions here, please see my reply below. On 5/6/2015 10:32 PM, Suravee Suthikulanit wrote: > Hi All, > > 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. OUTPUT from lspci -tv: -[0000:00]-+-00.0 Advanced Micro Devices, Inc. [AMD] Device 1a00 +-02.0 Advanced Micro Devices, Inc. [AMD] Device 1a01 \-02.1 PCI Bridge -[01]--+-00.0 Intel Corporation 82599ES 10-Gigabit SFI/SFP+ Network Connection \-00.1 Intel Corporation 82599ES 10-Gigabit SFI/SFP+ Network Connection OUTPUT from lspci -vvv: 00:00.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Device 1a00 Subsystem: Advanced Micro Devices, Inc. [AMD] Device 1a00 Control: I/O- Mem- BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx- Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 0 00:02.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Device 1a01 Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx- Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- 00:02.1 PCI bridge: Advanced Micro Devices, Inc. [AMD] Device 1a02 (prog-if 00 [Normal decode]) Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx- Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 0 Bus: primary=00, secondary=01, subordinate=01, sec-latency=0 I/O behind bridge: 00fff000-00000fff Memory behind bridge: fff00000-000fffff Prefetchable memory behind bridge: 0000007fffe00000-0000007fffffffff Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort+ <SERR- <PERR- BridgeCtl: Parity- SERR- NoISA- VGA- MAbort- >Reset- FastB2B- PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn- ....... 01:00.0 Ethernet controller: Intel Corporation 82599ES 10-Gigabit SFI/SFP+ Network Connection (rev 01) Subsystem: Intel Corporation Ethernet Server Adapter X520-2 Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+ Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 0 Interrupt: pin A routed to IRQ 46 Region 0: Memory at 7fffe80000 (64-bit, prefetchable) [size=512K] Region 2: I/O ports at 0020 [size=32] Region 4: Memory at 7ffff04000 (64-bit, prefetchable) [size=16K] ....... 01:00.1 Ethernet controller: Intel Corporation 82599ES 10-Gigabit SFI/SFP+ Network Connection (rev 01) Subsystem: Intel Corporation Ethernet Server Adapter X520-2 Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+ Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 0 Interrupt: pin B routed to IRQ 47 Region 0: Memory at 7fffe00000 (64-bit, prefetchable) [size=512K] Region 2: I/O ports at 0000 [size=32] Region 4: Memory at 7ffff00000 (64-bit, prefetchable) [size=16K] ........ Thanks, Suravee
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. > OUTPUT from lspci -tv: > > -[0000:00]-+-00.0 Advanced Micro Devices, Inc. [AMD] Device 1a00 > +-02.0 Advanced Micro Devices, Inc. [AMD] Device 1a01 > \-02.1 PCI Bridge -[01]--+-00.0 Intel Corporation > 82599ES 10-Gigabit SFI/SFP+ Network Connection > \-00.1 Intel Corporation 82599ES 10-Gigabit > SFI/SFP+ Network Connection > > OUTPUT from lspci -vvv: > > 00:00.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Device 1a00 > Subsystem: Advanced Micro Devices, Inc. [AMD] Device 1a00 > Control: I/O- Mem- BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- > Stepping- SERR- FastB2B- DisINTx- > Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- > <TAbort- <MAbort- >SERR- <PERR- INTx- > Latency: 0 > > 00:02.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Device 1a01 > Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- > Stepping- SERR- FastB2B- DisINTx- > Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- > <TAbort- <MAbort- >SERR- <PERR- INTx- > > 00:02.1 PCI bridge: Advanced Micro Devices, Inc. [AMD] Device 1a02 > (prog-if 00 [Normal decode]) > Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- > Stepping- SERR- FastB2B- DisINTx- > Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- > <TAbort- <MAbort- >SERR- <PERR- INTx- > Latency: 0 > Bus: primary=00, secondary=01, subordinate=01, sec-latency=0 > I/O behind bridge: 00fff000-00000fff > Memory behind bridge: fff00000-000fffff > Prefetchable memory behind bridge: 0000007fffe00000-0000007fffffffff > Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- > <TAbort- <MAbort+ <SERR- <PERR- > BridgeCtl: Parity- SERR- NoISA- VGA- MAbort- >Reset- FastB2B- > PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn- > ....... > > 01:00.0 Ethernet controller: Intel Corporation 82599ES 10-Gigabit > SFI/SFP+ Network Connection (rev 01) > Subsystem: Intel Corporation Ethernet Server Adapter X520-2 > Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- > Stepping- SERR- FastB2B- DisINTx+ > Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- > <TAbort- <MAbort- >SERR- <PERR- INTx- > Latency: 0 > Interrupt: pin A routed to IRQ 46 > Region 0: Memory at 7fffe80000 (64-bit, prefetchable) [size=512K] > Region 2: I/O ports at 0020 [size=32] > Region 4: Memory at 7ffff04000 (64-bit, prefetchable) [size=16K] > ....... > > 01:00.1 Ethernet controller: Intel Corporation 82599ES 10-Gigabit > SFI/SFP+ Network Connection (rev 01) > Subsystem: Intel Corporation Ethernet Server Adapter X520-2 > Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- > Stepping- SERR- FastB2B- DisINTx+ > Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- > <TAbort- <MAbort- >SERR- <PERR- INTx- > Latency: 0 > Interrupt: pin B routed to IRQ 47 > Region 0: Memory at 7fffe00000 (64-bit, prefetchable) [size=512K] > Region 2: I/O ports at 0000 [size=32] > Region 4: Memory at 7ffff00000 (64-bit, prefetchable) [size=16K] > ........ > > Thanks, > Suravee >
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. Since you mentioned in a separate reply in this thread that we might not want to be using pci_claim_one_bus(), I guess we probably should drop this approach for now. Thanks, Suravee
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
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); @@ -136,6 +148,36 @@ int pci_claim_resource(struct pci_dev *dev, int resource) } EXPORT_SYMBOL(pci_claim_resource); +void pci_claim_one_bus(struct pci_bus *b) +{ + struct pci_dev *pdev; + struct pci_bus *child_bus; + + list_for_each_entry(pdev, &b->devices, bus_list) { + int i; + + for (i = 0; i < PCI_NUM_RESOURCES; i++) { + struct resource *r = &pdev->resource[i]; + + if (r->parent || !r->start || !r->flags) + continue; + + if (pci_has_flag(PCI_PROBE_ONLY) || + (r->flags & IORESOURCE_PCI_FIXED)) { + if (pci_claim_resource(pdev, i) == 0) + continue; + + pci_claim_bridge_resource(pdev, i); + } + } + } + + list_for_each_entry(child_bus, &b->children, node) { + pci_claim_one_bus(child_bus); + } +} +EXPORT_SYMBOL(pci_claim_one_bus); + void pci_disable_bridge_window(struct pci_dev *dev) { dev_info(&dev->dev, "disabling bridge mem windows\n"); diff --git a/include/linux/pci.h b/include/linux/pci.h index 353db8d..b59ad4b 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1085,6 +1085,7 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge); void pci_assign_unassigned_bus_resources(struct pci_bus *bus); void pci_assign_unassigned_root_bus_resources(struct pci_bus *bus); void pdev_enable_device(struct pci_dev *); +void pci_claim_one_bus(struct pci_bus *b); int pci_enable_resources(struct pci_dev *, int mask); void pci_fixup_irqs(u8 (*)(struct pci_dev *, u8 *),