[v3,00/10] ARM: tegra: Add PCIe device tree support

Commit Message

Thierry Reding Aug. 13, 2012, 5:40 p.m. UTC

On Mon, Aug 06, 2012 at 01:42:21PM -0600, Stephen Warren wrote:
> On 07/26/2012 01:55 PM, Thierry Reding wrote:
> > This patch series adds support for device tree based probing of the PCIe
> > controller found on Tegra SoCs.
> 
> Thierry, I just tested all Tegra boards in v3.6-rc1, and noticed that
> PCIe doesn't work on TrimSlice when booting use device tree. I think I
> found the cause, and I can't see why the same problem doesn't affect
> this series. Perhaps you can enlighten me?
> 
> When booting TrimSlice (or Harmony) using board files, Tegra's PCIe is
> initialized using a subsys_initcall to tegra_pcie_init() directly (or
> for Harmony to harmony_pcie_init() which then calls tegra_pcie_init()).
> 
> The final thing tegra_pcie_init() does is call pci_common_init(). This
> calls pcibios_init_hw() which calls hw->scan() which calls
> pci_scan_root_bus() which adds a device object for each device on the
> PCIe bus. However, since this happens very early in the boot sequence, I
> believe the enumerated PCIe devices don't immediately get probed.
> Instead, control gets returned to pci_common_init() which I believe then
> calls pci_bus_assign_resources() which actually sets up the resources
> for those devices. Later, the PCIe devices actually get probed, and
> everything works.
> 
> However, when booting using device tree, with the code currently in
> v3.6-rc1, tegra_pcie_init() is called late in the boot sequence, and so
> in the sequence described above, as soon as pci_scan_root_bus() adds a
> device, it gets probed, before the device object's resources have been
> set up, which results in the following failure:
> 
> PCI: Device 0000:01:00.0 not available because of resource collisions
> 
> ... because of the following code in pcibios_enable_device():
> 
> > 	for (idx = 0; idx < 6; idx++) {
> > 		/* Only set up the requested stuff */
> > 		if (!(mask & (1 << idx)))
> > 			continue;
> > 
> > 		r = dev->resource + idx;
> > 		if (!r->start && r->end) {
> > 			printk(KERN_ERR "PCI: Device %s not available because"
> > 			       " of resource collisions\n", pci_name(dev));
> 
> Doesn't this same problem exist when instantiating the PCIe device
> itself from device tree as in your patch series? If not, can you explain
> why?
> 
> Now, the obvious solution in v3.6 would be to simply have
> tegra_pcie_init() be called at the same early stage in the boot process
> when booting using device tree as it is when booting using board files.
> This works for TrimSlice.
> 
> However, on Harmony, it doesn't work, because PCIe on Harmony depends on
> regulators, and the regulators are accessed using an I2C bus that is
> instantiated from DT, and the instantiation of the I2C bus happens
> fairly late in the boot process so can't be found early during the boot
> sequence. See harmony_regulator_init() for the failing code.
> 
> Does anyone have any good ideas (small, self-contained patches) for
> solving this in v3.6 in such a way that PCIe works on both TrimSlice and
> Harmony?
> 
> Thanks.

I've looked into this a bit, and it seems like ARM is using an open-
coded version of the pci_enable_resources() function here, with the only
difference being the unconditional enabling of both I/O and memory-
mapped access for bridges. On Tegra there is already a PCI fixup to do
this, so pci_enable_resources() can be used as-is. I came up with the
attached patch but haven't been able to test it yet.

Thierry
From ebd69ae0a3d076e574da74d963cb3834b71dc6ad Mon Sep 17 00:00:00 2001
From: Thierry Reding <[e-mail]>
Date: Mon, 13 Aug 2012 18:49:28 +0200
Subject: [PATCH] ARM: PCI: refactor pcibios_enable_device()

The implementation is an open-coded version on pci_enable_resources()
with a special case to enable I/O and memory-mapped functionality on
bridges. This commit reuses the existing PCI core implementation of the
pci_enable_resources() function. This also means that bridges no longer
enable I/O and memory-mapped functionality unconditionally. Platforms
where this is really required can add a corresponding fixup.

Signed-off-by: Thierry Reding <[e-mail]>
---
 arch/arm/kernel/bios32.c | 36 +-----------------------------------
 1 file changed, 1 insertion(+), 35 deletions(-)

Comments

Stephen Warren Aug. 13, 2012, 6:47 p.m. UTC | #1

On 08/13/2012 11:40 AM, Thierry Reding wrote:
> On Mon, Aug 06, 2012 at 01:42:21PM -0600, Stephen Warren wrote:
>> On 07/26/2012 01:55 PM, Thierry Reding wrote:
>>> This patch series adds support for device tree based probing of
>>> the PCIe controller found on Tegra SoCs.
>> 
>> Thierry, I just tested all Tegra boards in v3.6-rc1, and noticed
>> that PCIe doesn't work on TrimSlice when booting use device tree.
>> I think I found the cause, and I can't see why the same problem
>> doesn't affect this series. Perhaps you can enlighten me?
...
>> PCI: Device 0000:01:00.0 not available because of resource
>> collisions
...
> I've looked into this a bit, and it seems like ARM is using an
> open- coded version of the pci_enable_resources() function here,
> with the only difference being the unconditional enabling of both
> I/O and memory- mapped access for bridges. On Tegra there is
> already a PCI fixup to do this, so pci_enable_resources() can be
> used as-is. I came up with the attached patch but haven't been able
> to test it yet.

Thanks very much for looking into this.

The patch did alter the behavior a little for TrimSlice, but didn't
solve the problem. The old error messages:

> [    2.173971] PCI: Device 0000:01:00.0 not available because of resource collisions
> [    2.181453] r8169 0000:01:00.0: (unregistered net_device): enable failure
> [    2.188254] r8169: probe of 0000:01:00.0 failed with error -22

Were replaced with the following with your patch:

> [    2.174010] r8169 0000:01:00.0: device not available (can't reserve [io  0x0000-0x00ff])
> [    2.182098] r8169 0000:01:00.0: (unregistered net_device): enable failure
> [    2.188900] r8169: probe of 0000:01:00.0 failed with error -22

This message appears from drivers/pci/setup-res.c pci_enable_resources()
due to:

> 		if (!r->parent) {
> 			dev_err(&dev->dev, "device not available "
> 				"(can't reserve %pR)\n", r);
> 			return -EINVAL;
> 		}

That check doesn't appear in ARM's custom pcibios_enable_device().
Disabling that check yields:

> [    2.174192] r8169 0000:01:00.0: enabling device (0140 -> 0143)
> [    2.180041] r8169 0000:01:00.0: BAR 2: can't reserve [mem 0x00000000-0x00000fff 64bit pref]
> [    2.188386] r8169 0000:01:00.0: (unregistered net_device): could not request regions
> [    2.196140] r8169: probe of 0000:01:00.0 failed with error -16

I think that's because the pci_dev's resources are initially assigned
PCI-aperture-relative addresses, and then these are later patched up to
take account of where the aperture is mapped into the CPU's address space.

Boot log using board files:

> [    1.146145] pci 0000:01:00.0: reg 10: [io  0x0000-0x00ff]
> [    1.151745] pci 0000:01:00.0: reg 18: [mem 0x00000000-0x00000fff 64bit pref]
> [    1.159007] pci 0000:01:00.0: reg 20: [mem 0x00000000-0x00003fff 64bit pref]
> [    1.166270] pci 0000:01:00.0: reg 30: [mem 0x00000000-0x0001ffff pref]
...
> [    1.217829] pci 0000:01:00.0: BAR 6: assigned [mem 0xa0000000-0xa001ffff pref]
> [    1.225264] pci 0000:01:00.0: BAR 4: assigned [mem 0xa0020000-0xa0023fff 64bit pref]
> [    1.233236] pci 0000:01:00.0: BAR 2: assigned [mem 0xa0024000-0xa0024fff 64bit pref]
> [    1.241206] pci 0000:01:00.0: BAR 0: assigned [io  0x1000-0x10ff]
... (I added some extra printks:)
> [    1.488007] r8169 0000:01:00.0: BAR 0: requesting [io  0x1000-0x10ff]
> [    1.501483] r8169 0000:01:00.0: BAR 2: requesting [mem 0xa0024000-0xa0024fff 64bit pref]
> [    1.516611] r8169 0000:01:00.0: BAR 4: requesting [mem 0xa0020000-0xa0023fff 64bit pref]

whereas for a device tree boot:

(same):
> [    2.112217] pci 0000:01:00.0: reg 10: [io  0x0000-0x00ff]
> [    2.117635] pci 0000:01:00.0: reg 18: [mem 0x00000000-0x00000fff 64bit pref]
> [    2.124690] pci 0000:01:00.0: reg 20: [mem 0x00000000-0x00003fff 64bit pref]
> [    2.131731] pci 0000:01:00.0: reg 30: [mem 0x00000000-0x0001ffff pref]
... (request region happens early)
> [    2.179838] r8169 0000:01:00.0: BAR 0: requesting [io  0x0000-0x00ff]
> [    2.193312] r8169 0000:01:00.0: BAR 2: requesting [mem 0x00000000-0x00000fff 64bit pref]
> [    2.201397] r8169 0000:01:00.0: BAR 2: can't reserve [mem 0x00000000-0x00000fff 64bit pref]
> [    2.209742] r8169 0000:01:00.0: (unregistered net_device): could not request regions
... (same, just happens too late)
> [    2.236818] pci 0000:01:00.0: BAR 6: assigned [mem 0xa0000000-0xa001ffff pref]
> [    2.244027] pci 0000:01:00.0: BAR 4: assigned [mem 0xa0020000-0xa0023fff 64bit pref]
> [    2.251794] pci 0000:01:00.0: BAR 2: assigned [mem 0xa0024000-0xa0024fff 64bit pref]
> [    2.259542] pci 0000:01:00.0: BAR 0: assigned [io  0x1000-0x10ff]

I suspect this is all still related to the PCI devices themselves being
probed much earlier in the overall PCI initialization sequence when the
PCI controller is probed later in the boot sequence, whereas PCI device
probe is deferred until the overall PCI initialization sequence is
complete if the PCI controller is probed very early in the boot sequence.

Does anyone know where/what that "probe now" vs. "probe later" decision
point is? I'll try and track it down if nobody beats me to it.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas Aug. 13, 2012, 11:18 p.m. UTC | #2

On Mon, Aug 13, 2012 at 11:47 AM, Stephen Warren <[e-mail]> wrote:
> On 08/13/2012 11:40 AM, Thierry Reding wrote:
>> On Mon, Aug 06, 2012 at 01:42:21PM -0600, Stephen Warren wrote:
>>> On 07/26/2012 01:55 PM, Thierry Reding wrote:
>>>> This patch series adds support for device tree based probing of
>>>> the PCIe controller found on Tegra SoCs.
>>>
>>> Thierry, I just tested all Tegra boards in v3.6-rc1, and noticed
>>> that PCIe doesn't work on TrimSlice when booting use device tree.
>>> I think I found the cause, and I can't see why the same problem
>>> doesn't affect this series. Perhaps you can enlighten me?
> ...
>>> PCI: Device 0000:01:00.0 not available because of resource
>>> collisions
> ...
>> I've looked into this a bit, and it seems like ARM is using an
>> open- coded version of the pci_enable_resources() function here,
>> with the only difference being the unconditional enabling of both
>> I/O and memory- mapped access for bridges. On Tegra there is
>> already a PCI fixup to do this, so pci_enable_resources() can be
>> used as-is.

I'd prefer that bridge I/O & memory access enabling be done in a
mainline path, not in a fixup.  Fixups are intended for working around
defects in specific devices, not for the normal path.  I know various
architectures have fixups that are used in the normal path, but I've
been working on eliminating them.

> The patch did alter the behavior a little for TrimSlice, but didn't
> solve the problem. The old error messages:
>
>> [    2.173971] PCI: Device 0000:01:00.0 not available because of resource collisions
>> [    2.181453] r8169 0000:01:00.0: (unregistered net_device): enable failure
>> [    2.188254] r8169: probe of 0000:01:00.0 failed with error -22
>
> Were replaced with the following with your patch:
>
>> [    2.174010] r8169 0000:01:00.0: device not available (can't reserve [io  0x0000-0x00ff])
>> [    2.182098] r8169 0000:01:00.0: (unregistered net_device): enable failure
>> [    2.188900] r8169: probe of 0000:01:00.0 failed with error -22
>
> This message appears from drivers/pci/setup-res.c pci_enable_resources()
> due to:
>
>>               if (!r->parent) {
>>                       dev_err(&dev->dev, "device not available "
>>                               "(can't reserve %pR)\n", r);
>>                       return -EINVAL;
>>               }
>
> That check doesn't appear in ARM's custom pcibios_enable_device().
> Disabling that check yields:
>
>> [    2.174192] r8169 0000:01:00.0: enabling device (0140 -> 0143)
>> [    2.180041] r8169 0000:01:00.0: BAR 2: can't reserve [mem 0x00000000-0x00000fff 64bit pref]
>> [    2.188386] r8169 0000:01:00.0: (unregistered net_device): could not request regions
>> [    2.196140] r8169: probe of 0000:01:00.0 failed with error -16
>
> I think that's because the pci_dev's resources are initially assigned
> PCI-aperture-relative addresses, and then these are later patched up to
> take account of where the aperture is mapped into the CPU's address space.

We definitely shouldn't be calling the driver probe routine before the
device BARs are assigned.

> Boot log using board files:
>
>> [    1.146145] pci 0000:01:00.0: reg 10: [io  0x0000-0x00ff]
>> [    1.151745] pci 0000:01:00.0: reg 18: [mem 0x00000000-0x00000fff 64bit pref]
>> [    1.159007] pci 0000:01:00.0: reg 20: [mem 0x00000000-0x00003fff 64bit pref]
>> [    1.166270] pci 0000:01:00.0: reg 30: [mem 0x00000000-0x0001ffff pref]
> ...
>> [    1.217829] pci 0000:01:00.0: BAR 6: assigned [mem 0xa0000000-0xa001ffff pref]
>> [    1.225264] pci 0000:01:00.0: BAR 4: assigned [mem 0xa0020000-0xa0023fff 64bit pref]
>> [    1.233236] pci 0000:01:00.0: BAR 2: assigned [mem 0xa0024000-0xa0024fff 64bit pref]
>> [    1.241206] pci 0000:01:00.0: BAR 0: assigned [io  0x1000-0x10ff]
> ... (I added some extra printks:)
>> [    1.488007] r8169 0000:01:00.0: BAR 0: requesting [io  0x1000-0x10ff]
>> [    1.501483] r8169 0000:01:00.0: BAR 2: requesting [mem 0xa0024000-0xa0024fff 64bit pref]
>> [    1.516611] r8169 0000:01:00.0: BAR 4: requesting [mem 0xa0020000-0xa0023fff 64bit pref]
>
> whereas for a device tree boot:
>
> (same):
>> [    2.112217] pci 0000:01:00.0: reg 10: [io  0x0000-0x00ff]
>> [    2.117635] pci 0000:01:00.0: reg 18: [mem 0x00000000-0x00000fff 64bit pref]
>> [    2.124690] pci 0000:01:00.0: reg 20: [mem 0x00000000-0x00003fff 64bit pref]
>> [    2.131731] pci 0000:01:00.0: reg 30: [mem 0x00000000-0x0001ffff pref]
> ... (request region happens early)
>> [    2.179838] r8169 0000:01:00.0: BAR 0: requesting [io  0x0000-0x00ff]
>> [    2.193312] r8169 0000:01:00.0: BAR 2: requesting [mem 0x00000000-0x00000fff 64bit pref]
>> [    2.201397] r8169 0000:01:00.0: BAR 2: can't reserve [mem 0x00000000-0x00000fff 64bit pref]
>> [    2.209742] r8169 0000:01:00.0: (unregistered net_device): could not request regions
> ... (same, just happens too late)
>> [    2.236818] pci 0000:01:00.0: BAR 6: assigned [mem 0xa0000000-0xa001ffff pref]
>> [    2.244027] pci 0000:01:00.0: BAR 4: assigned [mem 0xa0020000-0xa0023fff 64bit pref]
>> [    2.251794] pci 0000:01:00.0: BAR 2: assigned [mem 0xa0024000-0xa0024fff 64bit pref]
>> [    2.259542] pci 0000:01:00.0: BAR 0: assigned [io  0x1000-0x10ff]
>
> I suspect this is all still related to the PCI devices themselves being
> probed much earlier in the overall PCI initialization sequence when the
> PCI controller is probed later in the boot sequence, whereas PCI device
> probe is deferred until the overall PCI initialization sequence is
> complete if the PCI controller is probed very early in the boot sequence.

I don't know what to apply your patches to (they don't apply cleanly
to v3.6-rc2), so I can't see exactly what you're doing.  But it looks
like you might be calling pci_bus_add_devices() before
pci_bus_assign_resource(), which isn't going to work.

I don't know what it means to probe PCI devices before probing the PCI
controller (the host bridge) -- that shouldn't happen either.  In
order to probe PCI devices, we have to first know about the host
bridge so we know how to do config accesses and what the bridge
apertures are.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thierry Reding Aug. 14, 2012, 6:29 a.m. UTC | #3

On Mon, Aug 13, 2012 at 04:18:16PM -0700, Bjorn Helgaas wrote:
> On Mon, Aug 13, 2012 at 11:47 AM, Stephen Warren <[e-mail]> wrote:
> > On 08/13/2012 11:40 AM, Thierry Reding wrote:
> >> On Mon, Aug 06, 2012 at 01:42:21PM -0600, Stephen Warren wrote:
> >>> On 07/26/2012 01:55 PM, Thierry Reding wrote:
> >>>> This patch series adds support for device tree based probing of
> >>>> the PCIe controller found on Tegra SoCs.
> >>>
> >>> Thierry, I just tested all Tegra boards in v3.6-rc1, and noticed
> >>> that PCIe doesn't work on TrimSlice when booting use device tree.
> >>> I think I found the cause, and I can't see why the same problem
> >>> doesn't affect this series. Perhaps you can enlighten me?
> > ...
> >>> PCI: Device 0000:01:00.0 not available because of resource
> >>> collisions
> > ...
> >> I've looked into this a bit, and it seems like ARM is using an
> >> open- coded version of the pci_enable_resources() function here,
> >> with the only difference being the unconditional enabling of both
> >> I/O and memory- mapped access for bridges. On Tegra there is
> >> already a PCI fixup to do this, so pci_enable_resources() can be
> >> used as-is.
> 
> I'd prefer that bridge I/O & memory access enabling be done in a
> mainline path, not in a fixup.  Fixups are intended for working around
> defects in specific devices, not for the normal path.  I know various
> architectures have fixups that are used in the normal path, but I've
> been working on eliminating them.

I understand. Perhaps it should be added to the pci_enable_resources()
function?

> > The patch did alter the behavior a little for TrimSlice, but didn't
> > solve the problem. The old error messages:
> >
> >> [    2.173971] PCI: Device 0000:01:00.0 not available because of resource collisions
> >> [    2.181453] r8169 0000:01:00.0: (unregistered net_device): enable failure
> >> [    2.188254] r8169: probe of 0000:01:00.0 failed with error -22
> >
> > Were replaced with the following with your patch:
> >
> >> [    2.174010] r8169 0000:01:00.0: device not available (can't reserve [io  0x0000-0x00ff])
> >> [    2.182098] r8169 0000:01:00.0: (unregistered net_device): enable failure
> >> [    2.188900] r8169: probe of 0000:01:00.0 failed with error -22
> >
> > This message appears from drivers/pci/setup-res.c pci_enable_resources()
> > due to:
> >
> >>               if (!r->parent) {
> >>                       dev_err(&dev->dev, "device not available "
> >>                               "(can't reserve %pR)\n", r);
> >>                       return -EINVAL;
> >>               }
> >
> > That check doesn't appear in ARM's custom pcibios_enable_device().
> > Disabling that check yields:
> >
> >> [    2.174192] r8169 0000:01:00.0: enabling device (0140 -> 0143)
> >> [    2.180041] r8169 0000:01:00.0: BAR 2: can't reserve [mem 0x00000000-0x00000fff 64bit pref]
> >> [    2.188386] r8169 0000:01:00.0: (unregistered net_device): could not request regions
> >> [    2.196140] r8169: probe of 0000:01:00.0 failed with error -16
> >
> > I think that's because the pci_dev's resources are initially assigned
> > PCI-aperture-relative addresses, and then these are later patched up to
> > take account of where the aperture is mapped into the CPU's address space.
> 
> We definitely shouldn't be calling the driver probe routine before the
> device BARs are assigned.
> 
> > Boot log using board files:
> >
> >> [    1.146145] pci 0000:01:00.0: reg 10: [io  0x0000-0x00ff]
> >> [    1.151745] pci 0000:01:00.0: reg 18: [mem 0x00000000-0x00000fff 64bit pref]
> >> [    1.159007] pci 0000:01:00.0: reg 20: [mem 0x00000000-0x00003fff 64bit pref]
> >> [    1.166270] pci 0000:01:00.0: reg 30: [mem 0x00000000-0x0001ffff pref]
> > ...
> >> [    1.217829] pci 0000:01:00.0: BAR 6: assigned [mem 0xa0000000-0xa001ffff pref]
> >> [    1.225264] pci 0000:01:00.0: BAR 4: assigned [mem 0xa0020000-0xa0023fff 64bit pref]
> >> [    1.233236] pci 0000:01:00.0: BAR 2: assigned [mem 0xa0024000-0xa0024fff 64bit pref]
> >> [    1.241206] pci 0000:01:00.0: BAR 0: assigned [io  0x1000-0x10ff]
> > ... (I added some extra printks:)
> >> [    1.488007] r8169 0000:01:00.0: BAR 0: requesting [io  0x1000-0x10ff]
> >> [    1.501483] r8169 0000:01:00.0: BAR 2: requesting [mem 0xa0024000-0xa0024fff 64bit pref]
> >> [    1.516611] r8169 0000:01:00.0: BAR 4: requesting [mem 0xa0020000-0xa0023fff 64bit pref]
> >
> > whereas for a device tree boot:
> >
> > (same):
> >> [    2.112217] pci 0000:01:00.0: reg 10: [io  0x0000-0x00ff]
> >> [    2.117635] pci 0000:01:00.0: reg 18: [mem 0x00000000-0x00000fff 64bit pref]
> >> [    2.124690] pci 0000:01:00.0: reg 20: [mem 0x00000000-0x00003fff 64bit pref]
> >> [    2.131731] pci 0000:01:00.0: reg 30: [mem 0x00000000-0x0001ffff pref]
> > ... (request region happens early)
> >> [    2.179838] r8169 0000:01:00.0: BAR 0: requesting [io  0x0000-0x00ff]
> >> [    2.193312] r8169 0000:01:00.0: BAR 2: requesting [mem 0x00000000-0x00000fff 64bit pref]
> >> [    2.201397] r8169 0000:01:00.0: BAR 2: can't reserve [mem 0x00000000-0x00000fff 64bit pref]
> >> [    2.209742] r8169 0000:01:00.0: (unregistered net_device): could not request regions
> > ... (same, just happens too late)
> >> [    2.236818] pci 0000:01:00.0: BAR 6: assigned [mem 0xa0000000-0xa001ffff pref]
> >> [    2.244027] pci 0000:01:00.0: BAR 4: assigned [mem 0xa0020000-0xa0023fff 64bit pref]
> >> [    2.251794] pci 0000:01:00.0: BAR 2: assigned [mem 0xa0024000-0xa0024fff 64bit pref]
> >> [    2.259542] pci 0000:01:00.0: BAR 0: assigned [io  0x1000-0x10ff]
> >
> > I suspect this is all still related to the PCI devices themselves being
> > probed much earlier in the overall PCI initialization sequence when the
> > PCI controller is probed later in the boot sequence, whereas PCI device
> > probe is deferred until the overall PCI initialization sequence is
> > complete if the PCI controller is probed very early in the boot sequence.
> 
> I don't know what to apply your patches to (they don't apply cleanly
> to v3.6-rc2), so I can't see exactly what you're doing.  But it looks
> like you might be calling pci_bus_add_devices() before
> pci_bus_assign_resource(), which isn't going to work.

The patch was on top of this series, which in turn is based on -next.
However the patch doesn't actually change anything in the ordering of
the initialization sequence, it just replaces the ARM implementation
of pcibios_enable_device() to reuse pci_enable_resources().

ARM's main PCI entry point, arch/arm/kernel/bios32.c:pci_common_init(),
does call the correct sequence: pci_bus_assign_resources() followed by
pci_bus_add_devices(), so the sequence looks correct. However, judging
by the logs Stephen posted, the BAR assignment is too late, after the
driver has been probed.

> I don't know what it means to probe PCI devices before probing the PCI
> controller (the host bridge) -- that shouldn't happen either.  In
> order to probe PCI devices, we have to first know about the host
> bridge so we know how to do config accesses and what the bridge
> apertures are.

Right. I don't think this can happen. If the bridge hasn't been probed,
then the devices shouldn't be there anyway.

Thierry

Stephen Warren Aug. 14, 2012, 7:39 p.m. UTC | #4

On 08/13/2012 05:18 PM, Bjorn Helgaas wrote:
> On Mon, Aug 13, 2012 at 11:47 AM, Stephen Warren <[e-mail]> wrote:
...
>> whereas for a device tree boot:
>>
>> (same):
>>> [    2.112217] pci 0000:01:00.0: reg 10: [io  0x0000-0x00ff]
>>> [    2.117635] pci 0000:01:00.0: reg 18: [mem 0x00000000-0x00000fff 64bit pref]
>>> [    2.124690] pci 0000:01:00.0: reg 20: [mem 0x00000000-0x00003fff 64bit pref]
>>> [    2.131731] pci 0000:01:00.0: reg 30: [mem 0x00000000-0x0001ffff pref]
>> ... (request region happens early)
>>> [    2.179838] r8169 0000:01:00.0: BAR 0: requesting [io  0x0000-0x00ff]
>>> [    2.193312] r8169 0000:01:00.0: BAR 2: requesting [mem 0x00000000-0x00000fff 64bit pref]
>>> [    2.201397] r8169 0000:01:00.0: BAR 2: can't reserve [mem 0x00000000-0x00000fff 64bit pref]
>>> [    2.209742] r8169 0000:01:00.0: (unregistered net_device): could not request regions
>> ... (same, just happens too late)
>>> [    2.236818] pci 0000:01:00.0: BAR 6: assigned [mem 0xa0000000-0xa001ffff pref]
>>> [    2.244027] pci 0000:01:00.0: BAR 4: assigned [mem 0xa0020000-0xa0023fff 64bit pref]
>>> [    2.251794] pci 0000:01:00.0: BAR 2: assigned [mem 0xa0024000-0xa0024fff 64bit pref]
>>> [    2.259542] pci 0000:01:00.0: BAR 0: assigned [io  0x1000-0x10ff]
>>
>> I suspect this is all still related to the PCI devices themselves being
>> probed much earlier in the overall PCI initialization sequence when the
>> PCI controller is probed later in the boot sequence, whereas PCI device
>> probe is deferred until the overall PCI initialization sequence is
>> complete if the PCI controller is probed very early in the boot sequence.
> 
> I don't know what to apply your patches to (they don't apply cleanly
> to v3.6-rc2), so I can't see exactly what you're doing.  But it looks
> like you might be calling pci_bus_add_devices() before
> pci_bus_assign_resource(), which isn't going to work.

Yes, that's exactly what is happening.

PCIe initialization starts in arch/arm/mach-tegra/pci.e
tegra_pcie_init() which calls arch/arm/kernel/bios32.c
pci_common_init(). That function first calls pcibios_init_hw() (in the
same file, more about this later) and then loops over PCI buses, calling
amongst other things pci_bus_assign_resources() then pci_bus_add_devices().

The problem is that ARM's pcibios_init_hw() calls pci_scan_root_bus()
(or a host-driver-specific function which that also calls
pci_scan_root_bus() in Tegra's case) which in turn calls
pci_bus_add_devices() right at the end, before control has returned to
pci_common_init() and hence before pci_bus_assign_resources() has been
called.

If I modify pci_scan_root_bus() and remove the call to
pci_bus_add_devices(), everything works as expected.

So, I guess the question is: Should ARM's pcibios_init_hw() not be
calling pci_scan_root_bus(), or at least presumably the ARM PCI code
needs to do things in a slightly different order?
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thierry Reding Aug. 14, 2012, 7:58 p.m. UTC | #5

On Tue, Aug 14, 2012 at 01:39:23PM -0600, Stephen Warren wrote:
> On 08/13/2012 05:18 PM, Bjorn Helgaas wrote:
> > On Mon, Aug 13, 2012 at 11:47 AM, Stephen Warren <[e-mail]> wrote:
> ...
> >> whereas for a device tree boot:
> >>
> >> (same):
> >>> [    2.112217] pci 0000:01:00.0: reg 10: [io  0x0000-0x00ff]
> >>> [    2.117635] pci 0000:01:00.0: reg 18: [mem 0x00000000-0x00000fff 64bit pref]
> >>> [    2.124690] pci 0000:01:00.0: reg 20: [mem 0x00000000-0x00003fff 64bit pref]
> >>> [    2.131731] pci 0000:01:00.0: reg 30: [mem 0x00000000-0x0001ffff pref]
> >> ... (request region happens early)
> >>> [    2.179838] r8169 0000:01:00.0: BAR 0: requesting [io  0x0000-0x00ff]
> >>> [    2.193312] r8169 0000:01:00.0: BAR 2: requesting [mem 0x00000000-0x00000fff 64bit pref]
> >>> [    2.201397] r8169 0000:01:00.0: BAR 2: can't reserve [mem 0x00000000-0x00000fff 64bit pref]
> >>> [    2.209742] r8169 0000:01:00.0: (unregistered net_device): could not request regions
> >> ... (same, just happens too late)
> >>> [    2.236818] pci 0000:01:00.0: BAR 6: assigned [mem 0xa0000000-0xa001ffff pref]
> >>> [    2.244027] pci 0000:01:00.0: BAR 4: assigned [mem 0xa0020000-0xa0023fff 64bit pref]
> >>> [    2.251794] pci 0000:01:00.0: BAR 2: assigned [mem 0xa0024000-0xa0024fff 64bit pref]
> >>> [    2.259542] pci 0000:01:00.0: BAR 0: assigned [io  0x1000-0x10ff]
> >>
> >> I suspect this is all still related to the PCI devices themselves being
> >> probed much earlier in the overall PCI initialization sequence when the
> >> PCI controller is probed later in the boot sequence, whereas PCI device
> >> probe is deferred until the overall PCI initialization sequence is
> >> complete if the PCI controller is probed very early in the boot sequence.
> > 
> > I don't know what to apply your patches to (they don't apply cleanly
> > to v3.6-rc2), so I can't see exactly what you're doing.  But it looks
> > like you might be calling pci_bus_add_devices() before
> > pci_bus_assign_resource(), which isn't going to work.
> 
> Yes, that's exactly what is happening.
> 
> PCIe initialization starts in arch/arm/mach-tegra/pci.e
> tegra_pcie_init() which calls arch/arm/kernel/bios32.c
> pci_common_init(). That function first calls pcibios_init_hw() (in the
> same file, more about this later) and then loops over PCI buses, calling
> amongst other things pci_bus_assign_resources() then pci_bus_add_devices().
> 
> The problem is that ARM's pcibios_init_hw() calls pci_scan_root_bus()
> (or a host-driver-specific function which that also calls
> pci_scan_root_bus() in Tegra's case) which in turn calls
> pci_bus_add_devices() right at the end, before control has returned to
> pci_common_init() and hence before pci_bus_assign_resources() has been
> called.
> 
> If I modify pci_scan_root_bus() and remove the call to
> pci_bus_add_devices(), everything works as expected.
> 
> So, I guess the question is: Should ARM's pcibios_init_hw() not be
> calling pci_scan_root_bus(), or at least presumably the ARM PCI code
> needs to do things in a slightly different order?

Maybe pci_scan_root_bus() should be calling pci_bus_assign_resources()?
Or a new function could be added which also assigns the resources.

Thierry

Bjorn Helgaas Aug. 14, 2012, 9:55 p.m. UTC | #6

On Tue, Aug 14, 2012 at 12:58 PM, Thierry Reding
<[e-mail]> wrote:
> On Tue, Aug 14, 2012 at 01:39:23PM -0600, Stephen Warren wrote:
>> On 08/13/2012 05:18 PM, Bjorn Helgaas wrote:
>> > On Mon, Aug 13, 2012 at 11:47 AM, Stephen Warren <[e-mail]> wrote:
>> ...
>> >> whereas for a device tree boot:
>> >>
>> >> (same):
>> >>> [    2.112217] pci 0000:01:00.0: reg 10: [io  0x0000-0x00ff]
>> >>> [    2.117635] pci 0000:01:00.0: reg 18: [mem 0x00000000-0x00000fff 64bit pref]
>> >>> [    2.124690] pci 0000:01:00.0: reg 20: [mem 0x00000000-0x00003fff 64bit pref]
>> >>> [    2.131731] pci 0000:01:00.0: reg 30: [mem 0x00000000-0x0001ffff pref]
>> >> ... (request region happens early)
>> >>> [    2.179838] r8169 0000:01:00.0: BAR 0: requesting [io  0x0000-0x00ff]
>> >>> [    2.193312] r8169 0000:01:00.0: BAR 2: requesting [mem 0x00000000-0x00000fff 64bit pref]
>> >>> [    2.201397] r8169 0000:01:00.0: BAR 2: can't reserve [mem 0x00000000-0x00000fff 64bit pref]
>> >>> [    2.209742] r8169 0000:01:00.0: (unregistered net_device): could not request regions
>> >> ... (same, just happens too late)
>> >>> [    2.236818] pci 0000:01:00.0: BAR 6: assigned [mem 0xa0000000-0xa001ffff pref]
>> >>> [    2.244027] pci 0000:01:00.0: BAR 4: assigned [mem 0xa0020000-0xa0023fff 64bit pref]
>> >>> [    2.251794] pci 0000:01:00.0: BAR 2: assigned [mem 0xa0024000-0xa0024fff 64bit pref]
>> >>> [    2.259542] pci 0000:01:00.0: BAR 0: assigned [io  0x1000-0x10ff]
>> >>
>> >> I suspect this is all still related to the PCI devices themselves being
>> >> probed much earlier in the overall PCI initialization sequence when the
>> >> PCI controller is probed later in the boot sequence, whereas PCI device
>> >> probe is deferred until the overall PCI initialization sequence is
>> >> complete if the PCI controller is probed very early in the boot sequence.
>> >
>> > I don't know what to apply your patches to (they don't apply cleanly
>> > to v3.6-rc2), so I can't see exactly what you're doing.  But it looks
>> > like you might be calling pci_bus_add_devices() before
>> > pci_bus_assign_resource(), which isn't going to work.
>>
>> Yes, that's exactly what is happening.
>>
>> PCIe initialization starts in arch/arm/mach-tegra/pci.e
>> tegra_pcie_init() which calls arch/arm/kernel/bios32.c
>> pci_common_init(). That function first calls pcibios_init_hw() (in the
>> same file, more about this later) and then loops over PCI buses, calling
>> amongst other things pci_bus_assign_resources() then pci_bus_add_devices().
>>
>> The problem is that ARM's pcibios_init_hw() calls pci_scan_root_bus()
>> (or a host-driver-specific function which that also calls
>> pci_scan_root_bus() in Tegra's case) which in turn calls
>> pci_bus_add_devices() right at the end, before control has returned to
>> pci_common_init() and hence before pci_bus_assign_resources() has been
>> called.
>>
>> If I modify pci_scan_root_bus() and remove the call to
>> pci_bus_add_devices(), everything works as expected.
>>
>> So, I guess the question is: Should ARM's pcibios_init_hw() not be
>> calling pci_scan_root_bus(), or at least presumably the ARM PCI code
>> needs to do things in a slightly different order?

I think you need to do something like this instead of using pci_scan_root_bus():

    pci_create_root_bus()
    pci_scan_child_bus()
    pci_bus_assign_resources()
    pci_bus_add_devices()

This is the effective order used by most of the pci_create_root_bus() callers.

> Maybe pci_scan_root_bus() should be calling pci_bus_assign_resources()?
> Or a new function could be added which also assigns the resources.

Yes, it probably should.  I'm nervous about just throwing it in there
without quite a bit more analysis, but that's definitely the direction
I think we should be heading.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Warren Aug. 14, 2012, 10:58 p.m. UTC | #7

On 08/14/2012 03:55 PM, Bjorn Helgaas wrote:
> On Tue, Aug 14, 2012 at 12:58 PM, Thierry Reding
> <[e-mail]> wrote:
>> On Tue, Aug 14, 2012 at 01:39:23PM -0600, Stephen Warren wrote:
>>> On 08/13/2012 05:18 PM, Bjorn Helgaas wrote:
>>>> On Mon, Aug 13, 2012 at 11:47 AM, Stephen Warren <[e-mail]> wrote:
>>> ...
>>>>> whereas for a device tree boot:
>>>>>
>>>>> (same):
>>>>>> [    2.112217] pci 0000:01:00.0: reg 10: [io  0x0000-0x00ff]
>>>>>> [    2.117635] pci 0000:01:00.0: reg 18: [mem 0x00000000-0x00000fff 64bit pref]
>>>>>> [    2.124690] pci 0000:01:00.0: reg 20: [mem 0x00000000-0x00003fff 64bit pref]
>>>>>> [    2.131731] pci 0000:01:00.0: reg 30: [mem 0x00000000-0x0001ffff pref]
>>>>> ... (request region happens early)
>>>>>> [    2.179838] r8169 0000:01:00.0: BAR 0: requesting [io  0x0000-0x00ff]
>>>>>> [    2.193312] r8169 0000:01:00.0: BAR 2: requesting [mem 0x00000000-0x00000fff 64bit pref]
>>>>>> [    2.201397] r8169 0000:01:00.0: BAR 2: can't reserve [mem 0x00000000-0x00000fff 64bit pref]
>>>>>> [    2.209742] r8169 0000:01:00.0: (unregistered net_device): could not request regions
>>>>> ... (same, just happens too late)
>>>>>> [    2.236818] pci 0000:01:00.0: BAR 6: assigned [mem 0xa0000000-0xa001ffff pref]
>>>>>> [    2.244027] pci 0000:01:00.0: BAR 4: assigned [mem 0xa0020000-0xa0023fff 64bit pref]
>>>>>> [    2.251794] pci 0000:01:00.0: BAR 2: assigned [mem 0xa0024000-0xa0024fff 64bit pref]
>>>>>> [    2.259542] pci 0000:01:00.0: BAR 0: assigned [io  0x1000-0x10ff]
>>>>>
>>>>> I suspect this is all still related to the PCI devices themselves being
>>>>> probed much earlier in the overall PCI initialization sequence when the
>>>>> PCI controller is probed later in the boot sequence, whereas PCI device
>>>>> probe is deferred until the overall PCI initialization sequence is
>>>>> complete if the PCI controller is probed very early in the boot sequence.
>>>>
>>>> I don't know what to apply your patches to (they don't apply cleanly
>>>> to v3.6-rc2), so I can't see exactly what you're doing.  But it looks
>>>> like you might be calling pci_bus_add_devices() before
>>>> pci_bus_assign_resource(), which isn't going to work.
>>>
>>> Yes, that's exactly what is happening.
>>>
>>> PCIe initialization starts in arch/arm/mach-tegra/pci.e
>>> tegra_pcie_init() which calls arch/arm/kernel/bios32.c
>>> pci_common_init(). That function first calls pcibios_init_hw() (in the
>>> same file, more about this later) and then loops over PCI buses, calling
>>> amongst other things pci_bus_assign_resources() then pci_bus_add_devices().
>>>
>>> The problem is that ARM's pcibios_init_hw() calls pci_scan_root_bus()
>>> (or a host-driver-specific function which that also calls
>>> pci_scan_root_bus() in Tegra's case) which in turn calls
>>> pci_bus_add_devices() right at the end, before control has returned to
>>> pci_common_init() and hence before pci_bus_assign_resources() has been
>>> called.
>>>
>>> If I modify pci_scan_root_bus() and remove the call to
>>> pci_bus_add_devices(), everything works as expected.
>>>
>>> So, I guess the question is: Should ARM's pcibios_init_hw() not be
>>> calling pci_scan_root_bus(), or at least presumably the ARM PCI code
>>> needs to do things in a slightly different order?
> 
> I think you need to do something like this instead of using pci_scan_root_bus():
> 
>     pci_create_root_bus()
>     pci_scan_child_bus()
>     pci_bus_assign_resources()
>     pci_bus_add_devices()
> 
> This is the effective order used by most of the pci_create_root_bus() callers.

That would pretty much duplicate everything in pci_scan_root_bus(). That
might cause divergence down the road.

Can't we make the call to pci_bus_add_devices() optional in
pci_scan_root_bus() somehow; one of:

* Add a parameter to pci_scan_root_bus() controlling this.

(rather a large patch)

* Split pci_scan_root_bus() into pci_scan_root_bus() and
pci_scan_root_bus_no_add(), such that pci_scan_root_bus() is just a
wrapper that calls pci_scan_root_bus_no_add() then pci_bus_add_devices().

(very simple patch, and the new function can easily be used as/when it's
needed, e.g. enabled just for Tegra in 3.6 to reduce risk of regressions)

* Add a flag to struct pci_bus that requests pci_scan_root_bus() skip
the call to pci_bus_add_devices().

(a flag in the bus struct just for one function seems a little
circuitous, but perhaps OK)

* ifdef out the call to pci_bus_add_devices(), if building for ARM.

(very simple, and probably correct)

Actually, I'm not totally convinced some other archs shouldn't skip this
too; while I couldn't find any other arch that explicitly calls
pci_bus_assign_resources() and pci_bus_add_devices() after
pci_scan_root_bus(), I did see some that call
pci_assign_unassigned_resources() which seems like it might be due to a
similar situation?

* Add another pcibios_*() callback that pci_scan_root_bus() calls to
determine whether to call pci_bus_add_devices(), with default
implementation.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Warren Aug. 14, 2012, 11:51 p.m. UTC | #8

On 08/14/2012 04:58 PM, Stephen Warren wrote:
> On 08/14/2012 03:55 PM, Bjorn Helgaas wrote:
>> On Tue, Aug 14, 2012 at 12:58 PM, Thierry Reding
>> <[e-mail]> wrote:
>>> On Tue, Aug 14, 2012 at 01:39:23PM -0600, Stephen Warren wrote:
>>>> On 08/13/2012 05:18 PM, Bjorn Helgaas wrote:
>>>>> On Mon, Aug 13, 2012 at 11:47 AM, Stephen Warren <[e-mail]> wrote:
>>>> ...
>>>>>> whereas for a device tree boot:
>>>>>>
>>>>>> (same):
>>>>>>> [    2.112217] pci 0000:01:00.0: reg 10: [io  0x0000-0x00ff]
>>>>>>> [    2.117635] pci 0000:01:00.0: reg 18: [mem 0x00000000-0x00000fff 64bit pref]
>>>>>>> [    2.124690] pci 0000:01:00.0: reg 20: [mem 0x00000000-0x00003fff 64bit pref]
>>>>>>> [    2.131731] pci 0000:01:00.0: reg 30: [mem 0x00000000-0x0001ffff pref]
>>>>>> ... (request region happens early)
>>>>>>> [    2.179838] r8169 0000:01:00.0: BAR 0: requesting [io  0x0000-0x00ff]
>>>>>>> [    2.193312] r8169 0000:01:00.0: BAR 2: requesting [mem 0x00000000-0x00000fff 64bit pref]
>>>>>>> [    2.201397] r8169 0000:01:00.0: BAR 2: can't reserve [mem 0x00000000-0x00000fff 64bit pref]
>>>>>>> [    2.209742] r8169 0000:01:00.0: (unregistered net_device): could not request regions
>>>>>> ... (same, just happens too late)
>>>>>>> [    2.236818] pci 0000:01:00.0: BAR 6: assigned [mem 0xa0000000-0xa001ffff pref]
>>>>>>> [    2.244027] pci 0000:01:00.0: BAR 4: assigned [mem 0xa0020000-0xa0023fff 64bit pref]
>>>>>>> [    2.251794] pci 0000:01:00.0: BAR 2: assigned [mem 0xa0024000-0xa0024fff 64bit pref]
>>>>>>> [    2.259542] pci 0000:01:00.0: BAR 0: assigned [io  0x1000-0x10ff]
>>>>>>
>>>>>> I suspect this is all still related to the PCI devices themselves being
>>>>>> probed much earlier in the overall PCI initialization sequence when the
>>>>>> PCI controller is probed later in the boot sequence, whereas PCI device
>>>>>> probe is deferred until the overall PCI initialization sequence is
>>>>>> complete if the PCI controller is probed very early in the boot sequence.
>>>>>
>>>>> I don't know what to apply your patches to (they don't apply cleanly
>>>>> to v3.6-rc2), so I can't see exactly what you're doing.  But it looks
>>>>> like you might be calling pci_bus_add_devices() before
>>>>> pci_bus_assign_resource(), which isn't going to work.
>>>>
>>>> Yes, that's exactly what is happening.
>>>>
>>>> PCIe initialization starts in arch/arm/mach-tegra/pci.e
>>>> tegra_pcie_init() which calls arch/arm/kernel/bios32.c
>>>> pci_common_init(). That function first calls pcibios_init_hw() (in the
>>>> same file, more about this later) and then loops over PCI buses, calling
>>>> amongst other things pci_bus_assign_resources() then pci_bus_add_devices().
>>>>
>>>> The problem is that ARM's pcibios_init_hw() calls pci_scan_root_bus()
>>>> (or a host-driver-specific function which that also calls
>>>> pci_scan_root_bus() in Tegra's case) which in turn calls
>>>> pci_bus_add_devices() right at the end, before control has returned to
>>>> pci_common_init() and hence before pci_bus_assign_resources() has been
>>>> called.
>>>>
>>>> If I modify pci_scan_root_bus() and remove the call to
>>>> pci_bus_add_devices(), everything works as expected.
>>>>
>>>> So, I guess the question is: Should ARM's pcibios_init_hw() not be
>>>> calling pci_scan_root_bus(), or at least presumably the ARM PCI code
>>>> needs to do things in a slightly different order?
>>
>> I think you need to do something like this instead of using pci_scan_root_bus():
>>
>>     pci_create_root_bus()
>>     pci_scan_child_bus()
>>     pci_bus_assign_resources()
>>     pci_bus_add_devices()
>>
>> This is the effective order used by most of the pci_create_root_bus() callers.
> 
> That would pretty much duplicate everything in pci_scan_root_bus(). That
> might cause divergence down the road.
> 
> Can't we make the call to pci_bus_add_devices() optional in
> pci_scan_root_bus() somehow; one of:

Sigh, that turns out not to work correctly; it solves at least this part
of the problem when booting using device tree, but when booting using a
board file, it causes the IRQ number passed to the PCIe device to be
bogus:-(

I give up for now.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas Aug. 15, 2012, 12:08 a.m. UTC | #9

On Tue, Aug 14, 2012 at 3:58 PM, Stephen Warren <[e-mail]> wrote:
> On 08/14/2012 03:55 PM, Bjorn Helgaas wrote:
>> On Tue, Aug 14, 2012 at 12:58 PM, Thierry Reding
>> <[e-mail]> wrote:
>>> On Tue, Aug 14, 2012 at 01:39:23PM -0600, Stephen Warren wrote:
>>>> On 08/13/2012 05:18 PM, Bjorn Helgaas wrote:
>>>>> On Mon, Aug 13, 2012 at 11:47 AM, Stephen Warren <[e-mail]> wrote:
>>>> ...
>>>>>> whereas for a device tree boot:
>>>>>>
>>>>>> (same):
>>>>>>> [    2.112217] pci 0000:01:00.0: reg 10: [io  0x0000-0x00ff]
>>>>>>> [    2.117635] pci 0000:01:00.0: reg 18: [mem 0x00000000-0x00000fff 64bit pref]
>>>>>>> [    2.124690] pci 0000:01:00.0: reg 20: [mem 0x00000000-0x00003fff 64bit pref]
>>>>>>> [    2.131731] pci 0000:01:00.0: reg 30: [mem 0x00000000-0x0001ffff pref]
>>>>>> ... (request region happens early)
>>>>>>> [    2.179838] r8169 0000:01:00.0: BAR 0: requesting [io  0x0000-0x00ff]
>>>>>>> [    2.193312] r8169 0000:01:00.0: BAR 2: requesting [mem 0x00000000-0x00000fff 64bit pref]
>>>>>>> [    2.201397] r8169 0000:01:00.0: BAR 2: can't reserve [mem 0x00000000-0x00000fff 64bit pref]
>>>>>>> [    2.209742] r8169 0000:01:00.0: (unregistered net_device): could not request regions
>>>>>> ... (same, just happens too late)
>>>>>>> [    2.236818] pci 0000:01:00.0: BAR 6: assigned [mem 0xa0000000-0xa001ffff pref]
>>>>>>> [    2.244027] pci 0000:01:00.0: BAR 4: assigned [mem 0xa0020000-0xa0023fff 64bit pref]
>>>>>>> [    2.251794] pci 0000:01:00.0: BAR 2: assigned [mem 0xa0024000-0xa0024fff 64bit pref]
>>>>>>> [    2.259542] pci 0000:01:00.0: BAR 0: assigned [io  0x1000-0x10ff]
>>>>>>
>>>>>> I suspect this is all still related to the PCI devices themselves being
>>>>>> probed much earlier in the overall PCI initialization sequence when the
>>>>>> PCI controller is probed later in the boot sequence, whereas PCI device
>>>>>> probe is deferred until the overall PCI initialization sequence is
>>>>>> complete if the PCI controller is probed very early in the boot sequence.
>>>>>
>>>>> I don't know what to apply your patches to (they don't apply cleanly
>>>>> to v3.6-rc2), so I can't see exactly what you're doing.  But it looks
>>>>> like you might be calling pci_bus_add_devices() before
>>>>> pci_bus_assign_resource(), which isn't going to work.
>>>>
>>>> Yes, that's exactly what is happening.
>>>>
>>>> PCIe initialization starts in arch/arm/mach-tegra/pci.e
>>>> tegra_pcie_init() which calls arch/arm/kernel/bios32.c
>>>> pci_common_init(). That function first calls pcibios_init_hw() (in the
>>>> same file, more about this later) and then loops over PCI buses, calling
>>>> amongst other things pci_bus_assign_resources() then pci_bus_add_devices().
>>>>
>>>> The problem is that ARM's pcibios_init_hw() calls pci_scan_root_bus()
>>>> (or a host-driver-specific function which that also calls
>>>> pci_scan_root_bus() in Tegra's case) which in turn calls
>>>> pci_bus_add_devices() right at the end, before control has returned to
>>>> pci_common_init() and hence before pci_bus_assign_resources() has been
>>>> called.
>>>>
>>>> If I modify pci_scan_root_bus() and remove the call to
>>>> pci_bus_add_devices(), everything works as expected.
>>>>
>>>> So, I guess the question is: Should ARM's pcibios_init_hw() not be
>>>> calling pci_scan_root_bus(), or at least presumably the ARM PCI code
>>>> needs to do things in a slightly different order?
>>
>> I think you need to do something like this instead of using pci_scan_root_bus():
>>
>>     pci_create_root_bus()
>>     pci_scan_child_bus()
>>     pci_bus_assign_resources()
>>     pci_bus_add_devices()
>>
>> This is the effective order used by most of the pci_create_root_bus() callers.
>
> That would pretty much duplicate everything in pci_scan_root_bus(). That
> might cause divergence down the road.

That's true, but it is what most other architectures do, and if you do
it the same way, we'll be able to converge things more easily later.

> Can't we make the call to pci_bus_add_devices() optional in
> pci_scan_root_bus() somehow; one of:
>
> * Add a parameter to pci_scan_root_bus() controlling this.
>
> (rather a large patch)
>
> * Split pci_scan_root_bus() into pci_scan_root_bus() and
> pci_scan_root_bus_no_add(), such that pci_scan_root_bus() is just a
> wrapper that calls pci_scan_root_bus_no_add() then pci_bus_add_devices().
>
> (very simple patch, and the new function can easily be used as/when it's
> needed, e.g. enabled just for Tegra in 3.6 to reduce risk of regressions)
>
> * Add a flag to struct pci_bus that requests pci_scan_root_bus() skip
> the call to pci_bus_add_devices().
>
> (a flag in the bus struct just for one function seems a little
> circuitous, but perhaps OK)
>
> * ifdef out the call to pci_bus_add_devices(), if building for ARM.
>
> (very simple, and probably correct)

I'd rather not add more variants of pci_scan_root_bus().  We already
have several very similar things in drivers/pci/probe.c:

  pci_scan_bus()
  pci_scan_bus_parented()
  pci_scan_root_bus()

And in addition, we have many callers of pci_create_root_bus() that
look very much like one of these.  I'm trying to consolidate all this,
but there's a fair amount of work, and I think the simplest thing is
to just use pci_create_root_bus() for now.

> Actually, I'm not totally convinced some other archs shouldn't skip this
> too; while I couldn't find any other arch that explicitly calls
> pci_bus_assign_resources() and pci_bus_add_devices() after
> pci_scan_root_bus(), I did see some that call
> pci_assign_unassigned_resources() which seems like it might be due to a
> similar situation?

Yes, that does sound like a similar problem.  In the past, resource
assignment has been pretty much separate from enumeration, with the
arch having the responsibility to enumerate, assign, then add devices.
 But that is error-prone and needlessly arch-specific, and I'd like to
pull it into generic code.  It's just not done yet :)

> * Add another pcibios_*() callback that pci_scan_root_bus() calls to
> determine whether to call pci_bus_add_devices(), with default
> implementation.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Warren Aug. 15, 2012, 7:04 p.m. UTC | #10

On 08/14/2012 05:51 PM, Stephen Warren wrote:
> On 08/14/2012 04:58 PM, Stephen Warren wrote:
...
>> Can't we make the call to pci_bus_add_devices() optional in
>> pci_scan_root_bus() somehow; one of:
> 
> Sigh, that turns out not to work correctly; it solves at least this part
> of the problem when booting using device tree, but when booting using a
> board file, it causes the IRQ number passed to the PCIe device to be
> bogus:-(
> 
> I give up for now.

I think the appropriate workaround for Tegra in 3.6 is to simply make
any drivers for PCIe-based devices be modules instead of built-in, as
Thierry hinted at much earlier in the thread. I've validated that the
Ethernet works just fine on TrimSlice with that change, booting v3.6-rc1
using either board files or device tree.

For 3.7, we should continue the discussion about a real fix; I'll look
into the change Bjorn requested and see if it works, although given that
hacking pci_scan_root_bus as described immediately previously in this
thread caused a regression when booting using a board file, and the fact
that board files are no longer supported on Tegra, I'm not too confident
in the outcome...
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thierry Reding Aug. 15, 2012, 8:09 p.m. UTC | #11

On Wed, Aug 15, 2012 at 01:04:20PM -0600, Stephen Warren wrote:
> On 08/14/2012 05:51 PM, Stephen Warren wrote:
> > On 08/14/2012 04:58 PM, Stephen Warren wrote:
> ...
> >> Can't we make the call to pci_bus_add_devices() optional in
> >> pci_scan_root_bus() somehow; one of:
> > 
> > Sigh, that turns out not to work correctly; it solves at least this part
> > of the problem when booting using device tree, but when booting using a
> > board file, it causes the IRQ number passed to the PCIe device to be
> > bogus:-(
> > 
> > I give up for now.
> 
> I think the appropriate workaround for Tegra in 3.6 is to simply make
> any drivers for PCIe-based devices be modules instead of built-in, as
> Thierry hinted at much earlier in the thread. I've validated that the
> Ethernet works just fine on TrimSlice with that change, booting v3.6-rc1
> using either board files or device tree.

That's certainly the easiest and least error-prone solution.

> For 3.7, we should continue the discussion about a real fix; I'll look
> into the change Bjorn requested and see if it works, although given that
> hacking pci_scan_root_bus as described immediately previously in this
> thread caused a regression when booting using a board file, and the fact
> that board files are no longer supported on Tegra, I'm not too confident
> in the outcome...

I don't understand this last part. If the problem is there when booting
from board files and the board files are removed, doesn't that remove
the problem as well? =)

Thierry

Stephen Warren Aug. 15, 2012, 8:11 p.m. UTC | #12

On 08/15/2012 02:09 PM, Thierry Reding wrote:
> On Wed, Aug 15, 2012 at 01:04:20PM -0600, Stephen Warren wrote:
>> On 08/14/2012 05:51 PM, Stephen Warren wrote:
>>> On 08/14/2012 04:58 PM, Stephen Warren wrote:
>> ...
>>>> Can't we make the call to pci_bus_add_devices() optional in 
>>>> pci_scan_root_bus() somehow; one of:
>>> 
>>> Sigh, that turns out not to work correctly; it solves at least
>>> this part of the problem when booting using device tree, but
>>> when booting using a board file, it causes the IRQ number
>>> passed to the PCIe device to be bogus:-(
>>> 
>>> I give up for now.
>> 
>> I think the appropriate workaround for Tegra in 3.6 is to simply
>> make any drivers for PCIe-based devices be modules instead of
>> built-in, as Thierry hinted at much earlier in the thread. I've
>> validated that the Ethernet works just fine on TrimSlice with
>> that change, booting v3.6-rc1 using either board files or device
>> tree.
> 
> That's certainly the easiest and least error-prone solution.
> 
>> For 3.7, we should continue the discussion about a real fix; I'll
>> look into the change Bjorn requested and see if it works,
>> although given that hacking pci_scan_root_bus as described
>> immediately previously in this thread caused a regression when
>> booting using a board file, and the fact that board files are no
>> longer supported on Tegra, I'm not too confident in the
>> outcome...
> 
> I don't understand this last part. If the problem is there when
> booting from board files and the board files are removed, doesn't
> that remove the problem as well? =)

It does for ARM SoCs/CPUs exclusively using device tree, but not all
ARM systems are converting to device tree, so the fact that Tegra has
makes it harder for me not to break anything else.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thierry Reding Aug. 15, 2012, 8:19 p.m. UTC | #13

On Wed, Aug 15, 2012 at 02:11:10PM -0600, Stephen Warren wrote:
> On 08/15/2012 02:09 PM, Thierry Reding wrote:
> > On Wed, Aug 15, 2012 at 01:04:20PM -0600, Stephen Warren wrote:
> >> On 08/14/2012 05:51 PM, Stephen Warren wrote:
> >>> On 08/14/2012 04:58 PM, Stephen Warren wrote:
> >> ...
> >>>> Can't we make the call to pci_bus_add_devices() optional in 
> >>>> pci_scan_root_bus() somehow; one of:
> >>> 
> >>> Sigh, that turns out not to work correctly; it solves at least
> >>> this part of the problem when booting using device tree, but
> >>> when booting using a board file, it causes the IRQ number
> >>> passed to the PCIe device to be bogus:-(
> >>> 
> >>> I give up for now.
> >> 
> >> I think the appropriate workaround for Tegra in 3.6 is to simply
> >> make any drivers for PCIe-based devices be modules instead of
> >> built-in, as Thierry hinted at much earlier in the thread. I've
> >> validated that the Ethernet works just fine on TrimSlice with
> >> that change, booting v3.6-rc1 using either board files or device
> >> tree.
> > 
> > That's certainly the easiest and least error-prone solution.
> > 
> >> For 3.7, we should continue the discussion about a real fix; I'll
> >> look into the change Bjorn requested and see if it works,
> >> although given that hacking pci_scan_root_bus as described
> >> immediately previously in this thread caused a regression when
> >> booting using a board file, and the fact that board files are no
> >> longer supported on Tegra, I'm not too confident in the
> >> outcome...
> > 
> > I don't understand this last part. If the problem is there when
> > booting from board files and the board files are removed, doesn't
> > that remove the problem as well? =)
> 
> It does for ARM SoCs/CPUs exclusively using device tree, but not all
> ARM systems are converting to device tree, so the fact that Tegra has
> makes it harder for me not to break anything else.

I think the best road to a real fix would be to implement a custom scan
function for Tegra along the lines of what Bjorn suggested. Ideally,
this implementation should eventually converge to what's done on other
architectures. At that point maybe ARM can completely be converted to
this new generic implementation and the Tegra-specific hook removed.

Thierry

Stephen Warren Sept. 7, 2012, 11:34 p.m. UTC | #14

On 08/14/2012 05:51 PM, Stephen Warren wrote:
> On 08/14/2012 04:58 PM, Stephen Warren wrote:
>> On 08/14/2012 03:55 PM, Bjorn Helgaas wrote:
>>> On Tue, Aug 14, 2012 at 12:58 PM, Thierry Reding
>>> <[e-mail]> wrote:
>>>> On Tue, Aug 14, 2012 at 01:39:23PM -0600, Stephen Warren wrote:
>>>>> On 08/13/2012 05:18 PM, Bjorn Helgaas wrote:
>>>>>> On Mon, Aug 13, 2012 at 11:47 AM, Stephen Warren <[e-mail]> wrote:
>>>>> ...
>>>>>>> whereas for a device tree boot:
>>>>>>>
>>>>>>> (same):
>>>>>>>> [    2.112217] pci 0000:01:00.0: reg 10: [io  0x0000-0x00ff]
>>>>>>>> [    2.117635] pci 0000:01:00.0: reg 18: [mem 0x00000000-0x00000fff 64bit pref]
>>>>>>>> [    2.124690] pci 0000:01:00.0: reg 20: [mem 0x00000000-0x00003fff 64bit pref]
>>>>>>>> [    2.131731] pci 0000:01:00.0: reg 30: [mem 0x00000000-0x0001ffff pref]
>>>>>>> ... (request region happens early)
>>>>>>>> [    2.179838] r8169 0000:01:00.0: BAR 0: requesting [io  0x0000-0x00ff]
>>>>>>>> [    2.193312] r8169 0000:01:00.0: BAR 2: requesting [mem 0x00000000-0x00000fff 64bit pref]
>>>>>>>> [    2.201397] r8169 0000:01:00.0: BAR 2: can't reserve [mem 0x00000000-0x00000fff 64bit pref]
>>>>>>>> [    2.209742] r8169 0000:01:00.0: (unregistered net_device): could not request regions
>>>>>>> ... (same, just happens too late)
>>>>>>>> [    2.236818] pci 0000:01:00.0: BAR 6: assigned [mem 0xa0000000-0xa001ffff pref]
>>>>>>>> [    2.244027] pci 0000:01:00.0: BAR 4: assigned [mem 0xa0020000-0xa0023fff 64bit pref]
>>>>>>>> [    2.251794] pci 0000:01:00.0: BAR 2: assigned [mem 0xa0024000-0xa0024fff 64bit pref]
>>>>>>>> [    2.259542] pci 0000:01:00.0: BAR 0: assigned [io  0x1000-0x10ff]
>>>>>>>
>>>>>>> I suspect this is all still related to the PCI devices themselves being
>>>>>>> probed much earlier in the overall PCI initialization sequence when the
>>>>>>> PCI controller is probed later in the boot sequence, whereas PCI device
>>>>>>> probe is deferred until the overall PCI initialization sequence is
>>>>>>> complete if the PCI controller is probed very early in the boot sequence.
>>>>>>
>>>>>> I don't know what to apply your patches to (they don't apply cleanly
>>>>>> to v3.6-rc2), so I can't see exactly what you're doing.  But it looks
>>>>>> like you might be calling pci_bus_add_devices() before
>>>>>> pci_bus_assign_resource(), which isn't going to work.
>>>>>
>>>>> Yes, that's exactly what is happening.
>>>>>
>>>>> PCIe initialization starts in arch/arm/mach-tegra/pci.e
>>>>> tegra_pcie_init() which calls arch/arm/kernel/bios32.c
>>>>> pci_common_init(). That function first calls pcibios_init_hw() (in the
>>>>> same file, more about this later) and then loops over PCI buses, calling
>>>>> amongst other things pci_bus_assign_resources() then pci_bus_add_devices().
>>>>>
>>>>> The problem is that ARM's pcibios_init_hw() calls pci_scan_root_bus()
>>>>> (or a host-driver-specific function which that also calls
>>>>> pci_scan_root_bus() in Tegra's case) which in turn calls
>>>>> pci_bus_add_devices() right at the end, before control has returned to
>>>>> pci_common_init() and hence before pci_bus_assign_resources() has been
>>>>> called.
>>>>>
>>>>> If I modify pci_scan_root_bus() and remove the call to
>>>>> pci_bus_add_devices(), everything works as expected.
>>>>>
>>>>> So, I guess the question is: Should ARM's pcibios_init_hw() not be
>>>>> calling pci_scan_root_bus(), or at least presumably the ARM PCI code
>>>>> needs to do things in a slightly different order?
>>>
>>> I think you need to do something like this instead of using pci_scan_root_bus():
>>>
>>>     pci_create_root_bus()
>>>     pci_scan_child_bus()
>>>     pci_bus_assign_resources()
>>>     pci_bus_add_devices()
>>>
>>> This is the effective order used by most of the pci_create_root_bus() callers.
>>
>> That would pretty much duplicate everything in pci_scan_root_bus(). That
>> might cause divergence down the road.
>>
>> Can't we make the call to pci_bus_add_devices() optional in
>> pci_scan_root_bus() somehow; one of:
> 
> Sigh, that turns out not to work correctly; it solves at least this part
> of the problem when booting using device tree, but when booting using a
> board file, it causes the IRQ number passed to the PCIe device to be
> bogus:-(

The reason this fails is because the following happens after
pci_scan_bus_root():

	pci_fixup_irqs(pcibios_swizzle, pcibios_map_irq);

(in arch/arm/kernel/bios32.c:pci_common_init())

So, if I replace the call to pci_scan_root_bus() with the code you wrote
above, then the IRQ numbers aren't assigned into the pci_dev structures
until after they're device_add()ed, and hence probed().

Ideally, I could just add a call to pci_fixup_irqs() right before the
call to pci_bus_add_devices() in the code you wrote above. However, that
won't work, because pci_fixup_irqs() finds all the PCI devices to fix up
by searching the list of devices that pci_bus_add_devices() adds to:-(

I guess it's a pretty basic premise of the current PCI code that all the
PCI scanning happens well before any device drivers are registered,
which in turn means that device_add() doesn't trigger the device's
probe() until much later, after all the fixups and resource assignments
are done?
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Russell King - ARM Linux Sept. 8, 2012, 12:04 a.m. UTC | #15

On Fri, Sep 07, 2012 at 05:34:35PM -0600, Stephen Warren wrote:
> I guess it's a pretty basic premise of the current PCI code that all the
> PCI scanning happens well before any device drivers are registered,
> which in turn means that device_add() doesn't trigger the device's
> probe() until much later, after all the fixups and resource assignments
> are done?

Are you saying that the PCI layer is again screwed up after all my
hard work several years ago to ensure that PCI devices are properly
setup _before_ they're made available to the PCI drivers then?  That
was around the time I was looking at Cardbus stuff, ensuring that that
worked with the same guarantees.

Not amused.

What is wrong with the "probe devices, apply fixups, setup resources,
apply more fixups, publish" process that it's had to be yet again
broken?
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Warren Sept. 8, 2012, 5:53 a.m. UTC | #16

On 09/07/2012 06:04 PM, Russell King - ARM Linux wrote:
> On Fri, Sep 07, 2012 at 05:34:35PM -0600, Stephen Warren wrote:
>> I guess it's a pretty basic premise of the current PCI code that all the
>> PCI scanning happens well before any device drivers are registered,
>> which in turn means that device_add() doesn't trigger the device's
>> probe() until much later, after all the fixups and resource assignments
>> are done?
> 
> Are you saying that the PCI layer is again screwed up after all my
> hard work several years ago to ensure that PCI devices are properly
> setup _before_ they're made available to the PCI drivers then?  That
> was around the time I was looking at Cardbus stuff, ensuring that that
> worked with the same guarantees.
> 
> Not amused.
> 
> What is wrong with the "probe devices, apply fixups, setup resources,
> apply more fixups, publish" process that it's had to be yet again
> broken?

I must admit, I'm having a hard time finding when the code worked like
that; ARM's bios32.c:pcibios_init_hw() seems to have always called
pci_scan_root_bus(), or ops function hw->scan(), which at least in the 1
PCIe controller driver I looked at, in turn always called either
pci_scan_root_bus() or another similar function that I believe always
called pci_bus_add_devices(), which is before pci_common_init() could
assign the resources. Maybe I'm just looking at the wrong PCIe
controller driver, or not looking back far enough in git history (or
pre-git)? If you could point out when it was working as you describe
(which sounds reasonable), I'd be interested in tracing the history from
there to see when/why it changed.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas Sept. 8, 2012, 5:51 p.m. UTC | #17

On Fri, Sep 7, 2012 at 6:04 PM, Russell King - ARM Linux
<[e-mail]> wrote:
> On Fri, Sep 07, 2012 at 05:34:35PM -0600, Stephen Warren wrote:
>> I guess it's a pretty basic premise of the current PCI code that all the
>> PCI scanning happens well before any device drivers are registered,
>> which in turn means that device_add() doesn't trigger the device's
>> probe() until much later, after all the fixups and resource assignments
>> are done?
>
> Are you saying that the PCI layer is again screwed up after all my
> hard work several years ago to ensure that PCI devices are properly
> setup _before_ they're made available to the PCI drivers then?  That
> was around the time I was looking at Cardbus stuff, ensuring that that
> worked with the same guarantees.
>
> Not amused.
>
> What is wrong with the "probe devices, apply fixups, setup resources,
> apply more fixups, publish" process that it's had to be yet again
> broken?

It seems that there are some bugs in the PCI layer, no doubt
introduced after all your hard work.  We'll do our best to fix them.

The particular issue of pci_fixup_irqs() has been on my list for a
while, and we talked about it at the recent PCI mini-summit.  It's
clearly broken that we do this with for_each_pci_dev() once at
boot-time because that does nothing for hot-added devices.  It's also
broken that it is called after device_add() because the core shouldn't
touch the device after it's available to drivers.

This should get fixed reasonably soon, probably not in v3.6, but
hopefully in v3.7.  It's not completely trivial because many arches
have the same problem, and we need to fix all of them.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thierry Reding Sept. 18, 2012, 6:33 a.m. UTC | #18

On Sat, Sep 08, 2012 at 11:51:00AM -0600, Bjorn Helgaas wrote:
> On Fri, Sep 7, 2012 at 6:04 PM, Russell King - ARM Linux
> <[e-mail]> wrote:
> > On Fri, Sep 07, 2012 at 05:34:35PM -0600, Stephen Warren wrote:
> >> I guess it's a pretty basic premise of the current PCI code that all the
> >> PCI scanning happens well before any device drivers are registered,
> >> which in turn means that device_add() doesn't trigger the device's
> >> probe() until much later, after all the fixups and resource assignments
> >> are done?
> >
> > Are you saying that the PCI layer is again screwed up after all my
> > hard work several years ago to ensure that PCI devices are properly
> > setup _before_ they're made available to the PCI drivers then?  That
> > was around the time I was looking at Cardbus stuff, ensuring that that
> > worked with the same guarantees.
> >
> > Not amused.
> >
> > What is wrong with the "probe devices, apply fixups, setup resources,
> > apply more fixups, publish" process that it's had to be yet again
> > broken?
> 
> It seems that there are some bugs in the PCI layer, no doubt
> introduced after all your hard work.  We'll do our best to fix them.
> 
> The particular issue of pci_fixup_irqs() has been on my list for a
> while, and we talked about it at the recent PCI mini-summit.  It's
> clearly broken that we do this with for_each_pci_dev() once at
> boot-time because that does nothing for hot-added devices.  It's also
> broken that it is called after device_add() because the core shouldn't
> touch the device after it's available to drivers.
> 
> This should get fixed reasonably soon, probably not in v3.6, but
> hopefully in v3.7.  It's not completely trivial because many arches
> have the same problem, and we need to fix all of them.

Has there been any progress on this? I've read the PCI mini summit notes
that you posted a while ago but it doesn't mention the pci_fixup_irqs()
issue. Was there some decision as to how this should be solved? If I
understand correctly this should solve the issue that Stephen has been
seeing with bogus interrupt assignments, so we'll need this to fix PCIe
on Tegra. Is there anything I can do to help move this forward?

Thierry

Bjorn Helgaas Sept. 18, 2012, 3:56 p.m. UTC | #19

On Tue, Sep 18, 2012 at 12:33 AM, Thierry Reding
<[e-mail]> wrote:
> On Sat, Sep 08, 2012 at 11:51:00AM -0600, Bjorn Helgaas wrote:
>> On Fri, Sep 7, 2012 at 6:04 PM, Russell King - ARM Linux
>> <[e-mail]> wrote:
>> > On Fri, Sep 07, 2012 at 05:34:35PM -0600, Stephen Warren wrote:
>> >> I guess it's a pretty basic premise of the current PCI code that all the
>> >> PCI scanning happens well before any device drivers are registered,
>> >> which in turn means that device_add() doesn't trigger the device's
>> >> probe() until much later, after all the fixups and resource assignments
>> >> are done?
>> >
>> > Are you saying that the PCI layer is again screwed up after all my
>> > hard work several years ago to ensure that PCI devices are properly
>> > setup _before_ they're made available to the PCI drivers then?  That
>> > was around the time I was looking at Cardbus stuff, ensuring that that
>> > worked with the same guarantees.
>> >
>> > Not amused.
>> >
>> > What is wrong with the "probe devices, apply fixups, setup resources,
>> > apply more fixups, publish" process that it's had to be yet again
>> > broken?
>>
>> It seems that there are some bugs in the PCI layer, no doubt
>> introduced after all your hard work.  We'll do our best to fix them.
>>
>> The particular issue of pci_fixup_irqs() has been on my list for a
>> while, and we talked about it at the recent PCI mini-summit.  It's
>> clearly broken that we do this with for_each_pci_dev() once at
>> boot-time because that does nothing for hot-added devices.  It's also
>> broken that it is called after device_add() because the core shouldn't
>> touch the device after it's available to drivers.
>>
>> This should get fixed reasonably soon, probably not in v3.6, but
>> hopefully in v3.7.  It's not completely trivial because many arches
>> have the same problem, and we need to fix all of them.
>
> Has there been any progress on this? I've read the PCI mini summit notes
> that you posted a while ago but it doesn't mention the pci_fixup_irqs()
> issue. Was there some decision as to how this should be solved? If I
> understand correctly this should solve the issue that Stephen has been
> seeing with bogus interrupt assignments, so we'll need this to fix PCIe
> on Tegra. Is there anything I can do to help move this forward?

I didn't mention pci_fixup_irqs() by name in the notes, but that's
part of what I meant by "device setup being done by initcalls" as a
hot-plug issue.  I know Myron Stowe expressed interest in working on
that, but I don't think he's had a chance to do anything yet (at
least, I haven't seen anything yet).

I don't have time to fix it myself, so moving it forward is just a
matter of somebody doing the work and posting the patches.  I think
that code just needs to be moved to some pcibios hook (possibly a new
one) that's called in the device_add path.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 13fd97b..dfe25f7 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -601,41 +601,7 @@  resource_size_t pcibios_align_resource(void *data, const struct resource *res,
  */
 int pcibios_enable_device(struct pci_dev *dev, int mask)
 {
-	u16 cmd, old_cmd;
-	int idx;
-	struct resource *r;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	old_cmd = cmd;
-	for (idx = 0; idx < 6; idx++) {
-		/* Only set up the requested stuff */
-		if (!(mask & (1 << idx)))
-			continue;
-
-		r = dev->resource + idx;
-		if (!r->start && r->end) {
-			printk(KERN_ERR "PCI: Device %s not available because"
-			       " of resource collisions\n", pci_name(dev));
-			return -EINVAL;
-		}
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-
-	/*
-	 * Bridges (eg, cardbus bridges) need to be fully enabled
-	 */
-	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE)
-		cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
-
-	if (cmd != old_cmd) {
-		printk("PCI: enabling device %s (%04x -> %04x)\n",
-		       pci_name(dev), old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	return 0;
+	return pci_enable_resources(dev, mask);
 }
 
 int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,