ARM: tegra: Add PCIe device tree support

When using deferred driver probing, PCI host controller drivers may actually require this function after the init stage.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- none

Changes in v2:
- use __devinit annotations

 drivers/pci/setup-irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/pci/setup-irq.c +2 -3

index eb219a1..f0bcd56 100644
@@ -18,7 +18,7 @@
 #include <linux/cache.h>
 
 
-static void __init
+static void __devinit
 pdev_fixup_irq(struct pci_dev *dev,
               u8 (*swizzle)(struct pci_dev *, u8 *),
               int (*map_irq)(const struct pci_dev *, u8, u8))

@@ -54,7 +54,7 @@ pdev_fixup_irq(struct pci_dev *dev,
        pcibios_update_irq(dev, irq);
 }
 
-void __init
+void __devinit
 pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 *),
               int (*map_irq)(const struct pci_dev *, u8, u8))
 {
--

1.7.11.2

Quoted 2 lines · Thierry Reding

When using deferred driver probing, PCI host controller drivers may actually require this function after the init stage.

Yes, this is a bug. Actually, there's still another bug here: if we hot-add a device, we won't do pdev_fixup_irq() for it. But your change is worthwhile even without fixing that other bug.

Normally I would include this in my PCI tree since it's under drivers/pci. That would make it easier for somebody to fix the hotplug problem in this cycle. Would having it in my PCI tree make things harder for you?

Quoted 1 line

Signed-off-by Thierry Reding <[email protected]>

Acked-by Bjorn Helgaas <[email protected]>

When using deferred driver probing, PCI host controller drivers may actually require this function after the init stage.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- none

Changes in v2:
- use __devinit annotations

 drivers/pci/setup-irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/pci/setup-irq.c +2 -3

index eb219a1..f0bcd56 100644
@@ -18,7 +18,7 @@
 #include <linux/cache.h>
 
 
-static void __init
+static void __devinit
 pdev_fixup_irq(struct pci_dev *dev,
               u8 (*swizzle)(struct pci_dev *, u8 *),
               int (*map_irq)(const struct pci_dev *, u8, u8))

@@ -54,7 +54,7 @@ pdev_fixup_irq(struct pci_dev *dev,
        pcibios_update_irq(dev, irq);
 }
 
-void __init
+void __devinit
 pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 *),
               int (*map_irq)(const struct pci_dev *, u8, u8))
 {
--

1.7.11.2

Quoted 10 lines · Thierry Reding

When using deferred driver probing, PCI host controller drivers may actually require this function after the init stage.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- none

Changes in v2:
- use __devinit annotations

Your original patch removed __init completely. Here you change it to __devinit. That means we decide whether to discard the function based on whether CONFIG_HOTPLUG is supported. But I think your point is not about hotplug; it's merely that we should be able to scan a PCI bus after init-time. We ought to be able to do a late PCI scan even if hotplug is not supported.

Therefore, I'd be inclined to remove __init completely unless you have another reason for preferring __devinit.

Quoted 20 lines · Bjorn Helgaas

Quoted 10 lines · Thierry Reding

When using deferred driver probing, PCI host controller drivers may actually require this function after the init stage.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- none

Changes in v2:
- use __devinit annotations

Your original patch removed __init completely. Here you change it to __devinit. That means we decide whether to discard the function based on whether CONFIG_HOTPLUG is supported. But I think your point is not about hotplug; it's merely that we should be able to scan a PCI bus after init-time. We ought to be able to do a late PCI scan even if hotplug is not supported.

Quoted 2 lines

Therefore, I'd be inclined to remove __init completely unless you have another reason for preferring __devinit.

I thought __devinit would resolve to nothing if HOTPLUG is defined and __init otherwise. That seemed more appropriate. However you are right that it is useful to always have it available, so I'm fine with removing the annotations altogether. Do you want me to follow up with a patch? Or can you just take the first version? I'm not sure if it still applies.

Quoted 21 lines · Thierry Reding

Quoted 20 lines · Bjorn Helgaas

Quoted 10 lines · Thierry Reding

When using deferred driver probing, PCI host controller drivers may actually require this function after the init stage.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- none

Changes in v2:
- use __devinit annotations

Your original patch removed __init completely. Here you change it to __devinit. That means we decide whether to discard the function based on whether CONFIG_HOTPLUG is supported. But I think your point is not about hotplug; it's merely that we should be able to scan a PCI bus after init-time. We ought to be able to do a late PCI scan even if hotplug is not supported.

Quoted 8 lines

Quoted 2 lines

Therefore, I'd be inclined to remove __init completely unless you have another reason for preferring __devinit.

I thought __devinit would resolve to nothing if HOTPLUG is defined and __init otherwise. That seemed more appropriate. However you are right that it is useful to always have it available, so I'm fine with removing the annotations altogether. Do you want me to follow up with a patch? Or can you just take the first version? I'm not sure if it still applies.

You're right about how __devinit works. It's just that I don't think hotplug is actually relevant here. We're trying to make pci_fixup_irqs() work after init, whether it's because of hotplug or simply because the arch scans host bridges after init.

I applied this to my "next" branch. Thanks!

Quoted 21 lines · Thierry Reding

Quoted 20 lines · Bjorn Helgaas

Quoted 10 lines · Thierry Reding

When using deferred driver probing, PCI host controller drivers may actually require this function after the init stage.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- none

Changes in v2:
- use __devinit annotations

Your original patch removed __init completely. Here you change it to __devinit. That means we decide whether to discard the function based on whether CONFIG_HOTPLUG is supported. But I think your point is not about hotplug; it's merely that we should be able to scan a PCI bus after init-time. We ought to be able to do a late PCI scan even if hotplug is not supported.

Quoted 8 lines

Quoted 2 lines

Therefore, I'd be inclined to remove __init completely unless you have another reason for preferring __devinit.

I thought __devinit would resolve to nothing if HOTPLUG is defined and __init otherwise. That seemed more appropriate. However you are right that it is useful to always have it available, so I'm fine with removing the annotations altogether. Do you want me to follow up with a patch? Or can you just take the first version? I'm not sure if it still applies.

You're right about how __devinit works. It's just that I don't think hotplug is actually relevant here. We're trying to make pci_fixup_irqs() work after init, whether it's because of hotplug or simply because the arch scans host bridges after init.

I applied this to my "next" branch. Thanks!

Quoted 37 lines · Bjorn Helgaas

Quoted 21 lines · Thierry Reding

Quoted 20 lines · Bjorn Helgaas

Quoted 10 lines · Thierry Reding

When using deferred driver probing, PCI host controller drivers may actually require this function after the init stage.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- none

Changes in v2:
- use __devinit annotations

Your original patch removed __init completely. Here you change it to __devinit. That means we decide whether to discard the function based on whether CONFIG_HOTPLUG is supported. But I think your point is not about hotplug; it's merely that we should be able to scan a PCI bus after init-time. We ought to be able to do a late PCI scan even if hotplug is not supported.

Quoted 8 lines

Quoted 2 lines

Therefore, I'd be inclined to remove __init completely unless you have another reason for preferring __devinit.

I thought __devinit would resolve to nothing if HOTPLUG is defined and __init otherwise. That seemed more appropriate. However you are right that it is useful to always have it available, so I'm fine with removing the annotations altogether. Do you want me to follow up with a patch? Or can you just take the first version? I'm not sure if it still applies.

You're right about how __devinit works. It's just that I don't think hotplug is actually relevant here. We're trying to make pci_fixup_irqs() work after init, whether it's because of hotplug or simply because the arch scans host bridges after init.

Quoted 1 line

I applied this to my "next" branch. Thanks!

Bjorn, I don't see this patch in next-20120907. Did it get dropped for some reason?

Quoted 38 lines · Stephen Warren

Quoted 37 lines · Bjorn Helgaas

Quoted 21 lines · Thierry Reding

Quoted 20 lines · Bjorn Helgaas

Quoted 10 lines · Thierry Reding

When using deferred driver probing, PCI host controller drivers may actually require this function after the init stage.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- none

Changes in v2:
- use __devinit annotations

Your original patch removed __init completely. Here you change it to __devinit. That means we decide whether to discard the function based on whether CONFIG_HOTPLUG is supported. But I think your point is not about hotplug; it's merely that we should be able to scan a PCI bus after init-time. We ought to be able to do a late PCI scan even if hotplug is not supported.

Quoted 8 lines

Quoted 2 lines

Therefore, I'd be inclined to remove __init completely unless you have another reason for preferring __devinit.

I thought __devinit would resolve to nothing if HOTPLUG is defined and __init otherwise. That seemed more appropriate. However you are right that it is useful to always have it available, so I'm fine with removing the annotations altogether. Do you want me to follow up with a patch? Or can you just take the first version? I'm not sure if it still applies.

You're right about how __devinit works. It's just that I don't think hotplug is actually relevant here. We're trying to make pci_fixup_irqs() work after init, whether it's because of hotplug or simply because the arch scans host bridges after init.

Quoted 4 lines

Quoted 1 line

I applied this to my "next" branch. Thanks!

Bjorn, I don't see this patch in next-20120907. Did it get dropped for some reason?

Yes, it turns out that dropping the annotations causes lots of section mismatches on other architectures. See here[0] for the details. I think the solution to the issue would be to either remove HOTPLUG altogether and drop __devinit and __devexit annotations or update all architectures to fix these warnings. I think Bjorn and I settled on the latter because it's obviously less intrusive. I've been busy building toolchains for all the PCI architectures and I think I have all of them. I'll just need some more time to build, find and fix any remaining section mismatches.

Quoted 44 lines · Thierry Reding

Quoted 38 lines · Stephen Warren

Quoted 37 lines · Bjorn Helgaas

Quoted 21 lines · Thierry Reding

Quoted 20 lines · Bjorn Helgaas

Quoted 10 lines · Thierry Reding

When using deferred driver probing, PCI host controller drivers may actually require this function after the init stage.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- none

Changes in v2:
- use __devinit annotations

Your original patch removed __init completely. Here you change it to __devinit. That means we decide whether to discard the function based on whether CONFIG_HOTPLUG is supported. But I think your point is not about hotplug; it's merely that we should be able to scan a PCI bus after init-time. We ought to be able to do a late PCI scan even if hotplug is not supported.

Quoted 8 lines

Quoted 2 lines

Therefore, I'd be inclined to remove __init completely unless you have another reason for preferring __devinit.

I thought __devinit would resolve to nothing if HOTPLUG is defined and __init otherwise. That seemed more appropriate. However you are right that it is useful to always have it available, so I'm fine with removing the annotations altogether. Do you want me to follow up with a patch? Or can you just take the first version? I'm not sure if it still applies.

You're right about how __devinit works. It's just that I don't think hotplug is actually relevant here. We're trying to make pci_fixup_irqs() work after init, whether it's because of hotplug or simply because the arch scans host bridges after init.

Quoted 4 lines

Quoted 1 line

I applied this to my "next" branch. Thanks!

Bjorn, I don't see this patch in next-20120907. Did it get dropped for some reason?

Quoted 8 lines

Yes, it turns out that dropping the annotations causes lots of section mismatches on other architectures. See here[0] for the details. I think the solution to the issue would be to either remove HOTPLUG altogether and drop __devinit and __devexit annotations or update all architectures to fix these warnings. I think Bjorn and I settled on the latter because it's obviously less intrusive. I've been busy building toolchains for all the PCI architectures and I think I have all of them. I'll just need some more time to build, find and fix any remaining section mismatches.

Greg KH is actively removing CONFIG_HOTPLUG altogether -- see
https://lkml.org/lkml/2012/9/4/489

That will make __devinit resolve to nothing in all cases, and we'll eventually remove __devinit completely. So we don't want to convert __init to __devinit; we have to remove the __init altogether. I think that means we have to update all arches first to avoid the section mismatches.

So I think Thierry is on the right track:
1) Change all arch pcibios_update_irq() implementations (and
probably a few other things) to be non-__init
2) Change pci_fixup_irqs() to be non-__init

Quoted 64 lines · Bjorn Helgaas

Quoted 44 lines · Thierry Reding

Quoted 38 lines · Stephen Warren

Quoted 37 lines · Bjorn Helgaas

Quoted 21 lines · Thierry Reding

Quoted 20 lines · Bjorn Helgaas

Quoted 10 lines · Thierry Reding

When using deferred driver probing, PCI host controller drivers may actually require this function after the init stage.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- none

Changes in v2:
- use __devinit annotations

Your original patch removed __init completely. Here you change it to __devinit. That means we decide whether to discard the function based on whether CONFIG_HOTPLUG is supported. But I think your point is not about hotplug; it's merely that we should be able to scan a PCI bus after init-time. We ought to be able to do a late PCI scan even if hotplug is not supported.

Quoted 8 lines

Quoted 2 lines

Therefore, I'd be inclined to remove __init completely unless you have another reason for preferring __devinit.

I thought __devinit would resolve to nothing if HOTPLUG is defined and __init otherwise. That seemed more appropriate. However you are right that it is useful to always have it available, so I'm fine with removing the annotations altogether. Do you want me to follow up with a patch? Or can you just take the first version? I'm not sure if it still applies.

You're right about how __devinit works. It's just that I don't think hotplug is actually relevant here. We're trying to make pci_fixup_irqs() work after init, whether it's because of hotplug or simply because the arch scans host bridges after init.

Quoted 4 lines

Quoted 1 line

I applied this to my "next" branch. Thanks!

Bjorn, I don't see this patch in next-20120907. Did it get dropped for some reason?

Quoted 8 lines

Yes, it turns out that dropping the annotations causes lots of section mismatches on other architectures. See here[0] for the details. I think the solution to the issue would be to either remove HOTPLUG altogether and drop __devinit and __devexit annotations or update all architectures to fix these warnings. I think Bjorn and I settled on the latter because it's obviously less intrusive. I've been busy building toolchains for all the PCI architectures and I think I have all of them. I'll just need some more time to build, find and fix any remaining section mismatches.

Greg KH is actively removing CONFIG_HOTPLUG altogether -- see
https://lkml.org/lkml/2012/9/4/489

That will make __devinit resolve to nothing in all cases, and we'll eventually remove __devinit completely. So we don't want to convert __init to __devinit; we have to remove the __init altogether. I think that means we have to update all arches first to avoid the section mismatches.

Quoted 4 lines

So I think Thierry is on the right track:
1) Change all arch pcibios_update_irq() implementations (and
probably a few other things) to be non-__init
2) Change pci_fixup_irqs() to be non-__init

Okay this wasn't as much work as I had thought. It turns out that there very few dependencies other than pcibios_update_irq(). Actually none at all. In addition some of the architectures already had these annotated with __devinit. Luckily those were the ones I wasn't able to build a cross-compiler for. Note that I resolved to converting all annotations from __init to __devinit first. I'll try to find out what exactly Greg is planning to do. If it turns out that the plan is to just remove the __devinit incrementally I could just drop all of these. Otherwise maybe a better option would be to leave them in and remove them all at once when the HOTPLUG symbol is removed.

Furthermore it seems like almost all implementations are the same, so I was going to include a patch that moves the common implementation to the PCI core and make it a weak symbol so architectures would still have the possibility to override it.

The only exception to this is 64-bit SPARC, which apparently doesn't do anything in pcibios_update_irq(). I wonder if it would hurt to just use the generic implementation anyway, which just sets a byte in the configuration space. That should work regardless of architecture, right?

Unicore32 and ARM also output a debugging message and maybe it would be okay to include that with the generic implementation as well. Do you have any objections?

This commit adds a new flag that allows marking resources as PCI configuration space.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- new patch

 include/linux/ioport.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/linux/ioport.h +0 -0

index 589e0e7..3314843 100644

include/linux/ioport.h +0 -1

 /* helpers to define resources */
 #define DEFINE_RES_NAMED(_start, _size, _name, _flags)                 \
--

1.7.11.2

Quoted 17 lines · Thierry Reding

This commit adds a new flag that allows marking resources as PCI configuration space.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- new patch

 include/linux/ioport.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/linux/ioport.h +0 -0

index 589e0e7..3314843 100644
@@ -102,7 +102,7 @@ struct resource {
 

include/linux/ioport.h struct resource { · line 105

Quoted 4 lines

include/linux/ioport.h +1 -1

 /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
 #define IORESOURCE_PCI_FIXED           (1<<4)  /* Do not move resource */
-
+#define IORESOURCE_PCI_CS              (1<<5)  /* PCI configuration space */

What is the purpose of this? It seems that you are marking regions that we call MMCONFIG on x86, or ECAM-type regions in the language of the PCIe spec. I see that you set it in several places, but I don't see anything that ever looks for it. Do you have plans to use it in the future? If it really does correspond to MMCONFIG/ECAM, we should handle those regions consistently across all architectures.

Quoted 19 lines · Bjorn Helgaas

Quoted 17 lines · Thierry Reding

This commit adds a new flag that allows marking resources as PCI configuration space.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- new patch

 include/linux/ioport.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/linux/ioport.h +0 -0

index 589e0e7..3314843 100644
@@ -102,7 +102,7 @@ struct resource {
 

Quoted 11 lines

include/linux/ioport.h struct resource { · line 105

Quoted 4 lines

include/linux/ioport.h +1 -1

 /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
 #define IORESOURCE_PCI_FIXED           (1<<4)  /* Do not move resource */
-
+#define IORESOURCE_PCI_CS              (1<<5)  /* PCI configuration space */

What is the purpose of this? It seems that you are marking regions that we call MMCONFIG on x86, or ECAM-type regions in the language of the PCIe spec. I see that you set it in several places, but I don't see anything that ever looks for it. Do you have plans to use it in the future? If it really does correspond to MMCONFIG/ECAM, we should handle those regions consistently across all architectures.

The purpose is ultimately to obtain the MMCONFIG/ECAM resources assigned to a PCI host controller. I've used this in the of_pci_parse_ranges() and in the static board setup code to mark ranges as such. Perhaps IORESOURCE_ECAM or IORESOURCE_MMCONFIG might have been better names. I also just noticed that I'm not using this anywhere, but the plan was to eventually use it with platform_get_resource(). However that doesn't seem to work either because the lower bits of the flags aren't use for comparison in that function.

Any other ideas how that could be handled? Basically what I need is a way to mark a resource as an MMCONFIG/ECAM range so that it can be used to program the PCI host controller accordingly. I don't know how these are assigned on x86. I was under the impression that the MMCONFIG/ECAM space was accessed through a single single address/data register pair.

Quoted 41 lines · Thierry Reding

Quoted 19 lines · Bjorn Helgaas

Quoted 17 lines · Thierry Reding

This commit adds a new flag that allows marking resources as PCI configuration space.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- new patch

 include/linux/ioport.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/linux/ioport.h +0 -0

index 589e0e7..3314843 100644
@@ -102,7 +102,7 @@ struct resource {
 

Quoted 11 lines

include/linux/ioport.h struct resource { · line 105

Quoted 4 lines

include/linux/ioport.h +1 -1

 /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
 #define IORESOURCE_PCI_FIXED           (1<<4)  /* Do not move resource */
-
+#define IORESOURCE_PCI_CS              (1<<5)  /* PCI configuration space */

What is the purpose of this? It seems that you are marking regions that we call MMCONFIG on x86, or ECAM-type regions in the language of the PCIe spec. I see that you set it in several places, but I don't see anything that ever looks for it. Do you have plans to use it in the future? If it really does correspond to MMCONFIG/ECAM, we should handle those regions consistently across all architectures.

The purpose is ultimately to obtain the MMCONFIG/ECAM resources assigned to a PCI host controller. I've used this in the of_pci_parse_ranges() and in the static board setup code to mark ranges as such. Perhaps IORESOURCE_ECAM or IORESOURCE_MMCONFIG might have been better names. I also just noticed that I'm not using this anywhere, but the plan was to eventually use it with platform_get_resource(). However that doesn't seem to work either because the lower bits of the flags aren't use for comparison in that function.

Quoted 5 lines

Any other ideas how that could be handled? Basically what I need is a way to mark a resource as an MMCONFIG/ECAM range so that it can be used to program the PCI host controller accordingly. I don't know how these are assigned on x86. I was under the impression that the MMCONFIG/ECAM space was accessed through a single single address/data register pair.

The legacy config access mechanism (CF8h/CFCh registers described in PCI 3.0 spec sec 3.2.2.3.2) is a single address/data pair, but this is mostly x86-specific. The ECAM mechanism (described in the PCIe 3.0 spec sec 7.2.2) is not a single address/data pair; instead, each byte of config space is directly mapped into the host's MMIO space.

Here's what we do on x86 (omitting some historical grunge that complicates things):

- Discover the host bridge via a PNP0A08 device in the ACPI namespace.
- Discover the bus number range behind the bridge using a _CRS
method in the PNP0A08 device.
- Discover the ECAM space for those buses via a _CBA method in the
PNP0A08 device.
- Tell the config accessors (struct pci_ops) that the ECAM space for
buses A-B is at address X.
- Enumerate the devices behind the host bridge by calling
pci_scan_root_bus(), passing the config accessors.

It sounds like you want a way to parse the resources at one point, saving them and marking the ECAM region, then at a later point, look up the ECAM from a saved list. We don't do that on x86 because the config accessors keep an internal list of the ECAM area for each bus.

We do of course want to put this ECAM space in the IORESOURCE_MEM tree because it consumes address space and we have to make sure we don't put anything else on top of it. But we don't have any reason to describe the MMIO -> config space function in that tree. From the point of view of the rest of the system, it's just MMIO space that's consumed by the PCI host bridge, just like any other device-specific MMIO area.

Quoted 73 lines · Bjorn Helgaas

Quoted 41 lines · Thierry Reding

Quoted 19 lines · Bjorn Helgaas

Quoted 17 lines · Thierry Reding

This commit adds a new flag that allows marking resources as PCI configuration space.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- new patch

 include/linux/ioport.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/linux/ioport.h +0 -0

index 589e0e7..3314843 100644
@@ -102,7 +102,7 @@ struct resource {
 

Quoted 11 lines

include/linux/ioport.h struct resource { · line 105

Quoted 4 lines

include/linux/ioport.h +1 -1

 /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
 #define IORESOURCE_PCI_FIXED           (1<<4)  /* Do not move resource */
-
+#define IORESOURCE_PCI_CS              (1<<5)  /* PCI configuration space */

What is the purpose of this? It seems that you are marking regions that we call MMCONFIG on x86, or ECAM-type regions in the language of the PCIe spec. I see that you set it in several places, but I don't see anything that ever looks for it. Do you have plans to use it in the future? If it really does correspond to MMCONFIG/ECAM, we should handle those regions consistently across all architectures.

The purpose is ultimately to obtain the MMCONFIG/ECAM resources assigned to a PCI host controller. I've used this in the of_pci_parse_ranges() and in the static board setup code to mark ranges as such. Perhaps IORESOURCE_ECAM or IORESOURCE_MMCONFIG might have been better names. I also just noticed that I'm not using this anywhere, but the plan was to eventually use it with platform_get_resource(). However that doesn't seem to work either because the lower bits of the flags aren't use for comparison in that function.

Quoted 5 lines

Any other ideas how that could be handled? Basically what I need is a way to mark a resource as an MMCONFIG/ECAM range so that it can be used to program the PCI host controller accordingly. I don't know how these are assigned on x86. I was under the impression that the MMCONFIG/ECAM space was accessed through a single single address/data register pair.

The legacy config access mechanism (CF8h/CFCh registers described in PCI 3.0 spec sec 3.2.2.3.2) is a single address/data pair, but this is mostly x86-specific. The ECAM mechanism (described in the PCIe 3.0 spec sec 7.2.2) is not a single address/data pair; instead, each byte of config space is directly mapped into the host's MMIO space.

Here's what we do on x86 (omitting some historical grunge that complicates things):

- Discover the host bridge via a PNP0A08 device in the ACPI namespace.
- Discover the bus number range behind the bridge using a _CRS
method in the PNP0A08 device.
- Discover the ECAM space for those buses via a _CBA method in the
PNP0A08 device.
- Tell the config accessors (struct pci_ops) that the ECAM space for
buses A-B is at address X.
- Enumerate the devices behind the host bridge by calling
pci_scan_root_bus(), passing the config accessors.

It sounds like you want a way to parse the resources at one point, saving them and marking the ECAM region, then at a later point, look up the ECAM from a saved list. We don't do that on x86 because the config accessors keep an internal list of the ECAM area for each bus.

Quoted 7 lines

We do of course want to put this ECAM space in the IORESOURCE_MEM tree because it consumes address space and we have to make sure we don't put anything else on top of it. But we don't have any reason to describe the MMIO -> config space function in that tree. From the point of view of the rest of the system, it's just MMIO space that's consumed by the PCI host bridge, just like any other device-specific MMIO area.

What I currently do is pass the ECAM space as a resource to the PCI host bridge platform device. Regular and extended configuration spaces are given by the third and fourth resources, respectively. If I understand correctly, you're saying that nothing beyond that needs to be encoded. In other words it is enough for the PCI host bridge driver to know where to take the data from.

I'll have to see what this means for the DT binding. There are other issues that I need to think about, like for example how to pass the ECAM space from the PCI host controller to each of the two bridges via the ranges property. This no longer makes sense in the current form, as the ECAM covers the configuration spaces for devices of both bridges and cannot really be split among them.

Quoted 82 lines · Thierry Reding

Quoted 73 lines · Bjorn Helgaas

Quoted 41 lines · Thierry Reding

Quoted 19 lines · Bjorn Helgaas

Quoted 17 lines · Thierry Reding

This commit adds a new flag that allows marking resources as PCI configuration space.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- new patch

 include/linux/ioport.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/linux/ioport.h +0 -0

index 589e0e7..3314843 100644
@@ -102,7 +102,7 @@ struct resource {
 

Quoted 11 lines

include/linux/ioport.h struct resource { · line 105

Quoted 4 lines

include/linux/ioport.h +1 -1

 /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
 #define IORESOURCE_PCI_FIXED           (1<<4)  /* Do not move resource */
-
+#define IORESOURCE_PCI_CS              (1<<5)  /* PCI configuration space */

What is the purpose of this? It seems that you are marking regions that we call MMCONFIG on x86, or ECAM-type regions in the language of the PCIe spec. I see that you set it in several places, but I don't see anything that ever looks for it. Do you have plans to use it in the future? If it really does correspond to MMCONFIG/ECAM, we should handle those regions consistently across all architectures.

The purpose is ultimately to obtain the MMCONFIG/ECAM resources assigned to a PCI host controller. I've used this in the of_pci_parse_ranges() and in the static board setup code to mark ranges as such. Perhaps IORESOURCE_ECAM or IORESOURCE_MMCONFIG might have been better names. I also just noticed that I'm not using this anywhere, but the plan was to eventually use it with platform_get_resource(). However that doesn't seem to work either because the lower bits of the flags aren't use for comparison in that function.

Quoted 5 lines

Any other ideas how that could be handled? Basically what I need is a way to mark a resource as an MMCONFIG/ECAM range so that it can be used to program the PCI host controller accordingly. I don't know how these are assigned on x86. I was under the impression that the MMCONFIG/ECAM space was accessed through a single single address/data register pair.

The legacy config access mechanism (CF8h/CFCh registers described in PCI 3.0 spec sec 3.2.2.3.2) is a single address/data pair, but this is mostly x86-specific. The ECAM mechanism (described in the PCIe 3.0 spec sec 7.2.2) is not a single address/data pair; instead, each byte of config space is directly mapped into the host's MMIO space.

Here's what we do on x86 (omitting some historical grunge that complicates things):

- Discover the host bridge via a PNP0A08 device in the ACPI namespace.
- Discover the bus number range behind the bridge using a _CRS
method in the PNP0A08 device.
- Discover the ECAM space for those buses via a _CBA method in the
PNP0A08 device.
- Tell the config accessors (struct pci_ops) that the ECAM space for
buses A-B is at address X.
- Enumerate the devices behind the host bridge by calling
pci_scan_root_bus(), passing the config accessors.

It sounds like you want a way to parse the resources at one point, saving them and marking the ECAM region, then at a later point, look up the ECAM from a saved list. We don't do that on x86 because the config accessors keep an internal list of the ECAM area for each bus.

Quoted 7 lines

We do of course want to put this ECAM space in the IORESOURCE_MEM tree because it consumes address space and we have to make sure we don't put anything else on top of it. But we don't have any reason to describe the MMIO -> config space function in that tree. From the point of view of the rest of the system, it's just MMIO space that's consumed by the PCI host bridge, just like any other device-specific MMIO area.

Quoted 6 lines

What I currently do is pass the ECAM space as a resource to the PCI host bridge platform device. Regular and extended configuration spaces are given by the third and fourth resources, respectively. If I understand correctly, you're saying that nothing beyond that needs to be encoded. In other words it is enough for the PCI host bridge driver to know where to take the data from.

Yes, I think so.

I'd like to someday make ECAM support generic, since it's specified by the PCIe spec. The spec doesn't differentiate between PCI configuration space (offsets 0-0xff) and PCI Express *extended* configuration space (offsets 0x100-0xfff). But it sounds like Tegra might have a memory-mapped configuration mechanism that is similar to ECAM but with a different address map, since you mention two resources (third and fourth). That's not a problem, since you're doing a Tegra-specific solution right now anyway, but something to keep in the back of your mind if we ever do make a generic ECAM solution.

Quoted 6 lines

I'll have to see what this means for the DT binding. There are other issues that I need to think about, like for example how to pass the ECAM space from the PCI host controller to each of the two bridges via the ranges property. This no longer makes sense in the current form, as the ECAM covers the configuration spaces for devices of both bridges and cannot really be split among them.

We have the same situation on x86. Part of the historical grunge that I omitted is that there's a static ACPI table (MCFG) that can tell you the ECAM range for a bus number range. Often that bus number range includes several host bridges. On x86, we have a single set of ECAM config accessors (see pci_mmcfg), and they maintain a list of "bus number -> ECAM addr" mappings internally, independent of which host bridge leads to the buses.

Stephen: Could you try to find out whether the regular configuration space translation can just be omitted if we already set up the one for the extended configuration space? In tegra_pcie_setup_translations(), BAR 0 is setup for regular configuration space (which requires a 16 MiB region), while BAR 1 is setup for the extended configuration space (requiring a full 256 MiB region). However, if I understand correctly, each of the registers that can be accessed via the BAR 0 translation can also be accessed via the BAR 1 translation. That seems like we're wasting the 16 MiB set aside for the BAR 0 mapping.

On 08/15/2012 12:49 AM, Thierry Reding wrote:
...

Quoted 10 lines

Stephen: Could you try to find out whether the regular configuration space translation can just be omitted if we already set up the one for the extended configuration space? In tegra_pcie_setup_translations(), BAR 0 is setup for regular configuration space (which requires a 16 MiB region), while BAR 1 is setup for the extended configuration space (requiring a full 256 MiB region). However, if I understand correctly, each of the registers that can be accessed via the BAR 0 translation can also be accessed via the BAR 1 translation. That seems like we're wasting the 16 MiB set aside for the BAR 0 mapping.

I have confirmed that in theory, the EXTCFG space can indeed be used to access any register, making the regular config space redundant. However, using the HW this way has apparently received less validation.

When a bus specifies #address-cells > 2, of_bus_default_map() now assumes that the mapping isn't for a physical address but rather an identifier that needs to match exactly.

This is required by bindings that use multiple cells to translate a resource to the parent bus (device index, type, ...).

See here for the discussion:

	https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-June/016577.html

Originally-by Arnd Bergmann <[email protected]>

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- new patch

 drivers/of/address.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/of/address.c +8 -0

index 7e262a6..2776119 100644
@@ -69,6 +69,14 @@ static u64 of_bus_default_map(u32 *addr, const __be32 *range,
 		 (unsigned long long)cp, (unsigned long long)s,
 		 (unsigned long long)da);
 
+	/*
+	 * If the number of address cells is larger than 2 we assume the
+	 * mapping doesn't specify a physical address. Rather, the address
+	 * specifies an identifier that must match exactly.
+	 */
+	if (na > 2 && memcmp(range, addr, na * 4) != 0)
+		return OF_BAD_ADDR;
+
 	if (da < cp || da >= (cp + s))
 		return OF_BAD_ADDR;
 	return da - cp;

Quoted 7 lines · Thierry Reding

When a bus specifies #address-cells > 2, of_bus_default_map() now assumes that the mapping isn't for a physical address but rather an identifier that needs to match exactly.

This is required by bindings that use multiple cells to translate a resource to the parent bus (device index, type, ...).

Quoted 6 lines

See here for the discussion:

	https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-June/016577.html

Originally-by Arnd Bergmann <[email protected]>

Signed-off-by Thierry Reding <[email protected]>

Acked-by Rob Herring <[email protected]>

Quoted 9 lines · Rob Herring

Quoted 8 lines · Thierry Reding

When a bus specifies #address-cells > 2, of_bus_default_map() now assumes that the mapping isn't for a physical address but rather an identifier that needs to match exactly.

This is required by bindings that use multiple cells to translate a resource to the parent bus (device index, type, ...).

Quoted 6 lines

Quoted 6 lines

See here for the discussion:

https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-June/016577.html

Quoted 4 lines

Quoted 2 lines

Signed-off-by: Thierry Reding
<[email protected]>

Acked-by Rob Herring <[email protected]>

Hi Rob,

Were you going to take this through your DT tree? I'm trying to reduce the number of patches in this series to make it more manageable and split it into smaller chunks. There are also a couple of issues that need to be resolved so I don't know if I can get the whole series into shape for 3.7.

However if you don't think this patch is useful to be applied by itself I can also carry it until the complete series is ready.

Quoted 10 lines · Thierry Reding

Quoted 9 lines · Rob Herring

Quoted 8 lines · Thierry Reding

When a bus specifies #address-cells > 2, of_bus_default_map() now assumes that the mapping isn't for a physical address but rather an identifier that needs to match exactly.

This is required by bindings that use multiple cells to translate a resource to the parent bus (device index, type, ...).

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

See here for the discussion:

https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-June/016577.html

Originally-by Arnd Bergmann <[email protected]>

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

Signed-off-by: Thierry Reding
<[email protected]>

Acked-by Rob Herring <[email protected]>

Hi Rob,

Quoted 8 lines

Were you going to take this through your DT tree? I'm trying to reduce the number of patches in this series to make it more manageable and split it into smaller chunks. There are also a couple of issues that need to be resolved so I don't know if I can get the whole series into shape for 3.7.

However if you don't think this patch is useful to be applied by itself I can also carry it until the complete series is ready.

Rob,

Are you able to take this patch now for 3.7, or should it be held off until the Tegra PCIe driver is re-written and requires this functionality (in which case I'd expect to take this through the Tegra tree at that time).

For reference, it's at http://patchwork.ozlabs.org/patch/173497/.

This new function parses the ranges property of PCI device nodes into an array of struct resource elements. It is useful in multiple-port PCI host controller drivers to collect information about the ranges that it needs to forward to the respective ports.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- new patch

 drivers/of/of_pci.c    | 84 +++++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/of_pci.h |  2 ++
 2 files changed, 85 insertions(+), 1 deletion(-)

drivers/of/of_pci.c +2 -1

index 13e37e2..bcff301 100644
@@ -1,7 +1,8 @@
 #include <linux/kernel.h>
 #include <linux/export.h>
-#include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_pci.h>
+#include <linux/slab.h>
 #include <asm/prom.h>
 
 static inline int __of_pci_pci_compare(struct device_node *node,

drivers/of/of_pci.c +14 -0

+	const __be32 *values;
+	int len, pna, np;
+
+	values = of_get_property(node, "ranges", &len);
+	if (!values)
+		return NULL;
+
+	pna = of_n_addr_cells(node);
+	np = pna + 5;
+
+	count = len / (np * sizeof(*values));
+
+	ranges = kzalloc(sizeof(*ranges) * count, GFP_KERNEL);
+	if (!ranges)

drivers/of/of_pci.c +51 -0

+
+		memset(&range, 0, sizeof(range));
+		range.start = addr;
+		range.end = addr + size - 1;
+
+		switch ((type >> 24) & 0x3) {
+		case 0:
+			range.flags = IORESOURCE_MEM | IORESOURCE_PCI_CS;
+			pr_debug("  CS  %#x-%#x\n", range.start, range.end);
+			break;
+
+		case 1:
+			range.flags = IORESOURCE_IO;
+			pr_debug("  IO  %#x-%#x\n", range.start, range.end);
+			break;
+
+		case 2:
+			range.flags = IORESOURCE_MEM;
+
+			if (type & 0x40000000) {
+				range.flags |= IORESOURCE_PREFETCH;
+				suffix = "prefetch";
+			}
+
+			pr_debug("  MEM %#x-%#x %s\n", range.start, range.end,
+				 suffix);
+			break;
+
+		case 3:
+			range.flags = IORESOURCE_MEM | IORESOURCE_MEM_64;
+
+			if (type & 0x40000000) {
+				range.flags |= IORESOURCE_PREFETCH;
+				suffix = "prefetch";
+			}
+
+			pr_debug("  MEM %#x-%#x 64-bit %s\n", range.start,
+				 range.end, suffix);
+			break;
+		}
+
+		memcpy(&ranges[i++], &range, sizeof(range));
+		values += np;
+	}
+
+	if (countp)
+		*countp = count;
+
+	return ranges;
+}
+EXPORT_SYMBOL_GPL(of_pci_parse_ranges);

include/linux/of_pci.h +2 -0

index bb115de..c0db6ea 100644
@@ -10,5 +10,7 @@ int of_irq_map_pci(const struct pci_dev *pdev, struct of_irq *out_irq);
 struct device_node;
 struct device_node *of_pci_find_child_device(struct device_node *parent,
 					     unsigned int devfn);
+struct resource *of_pci_parse_ranges(struct device_node *node,
+				     unsigned int *countp);
 
 #endif

Quoted 4 lines · Thierry Reding

This new function parses the ranges property of PCI device nodes into an array of struct resource elements. It is useful in multiple-port PCI host controller drivers to collect information about the ranges that it needs to forward to the respective ports.

It seems to me that some of the DT PCI code in arch/powerpc/kernel/pci* like pci_process_bridge_OF_ranges() should apply for ARM as well.

Each arch defining their own pci controller structs complicates this, but I would think at least the DT parsing can be common.

Quoted 22 lines

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- new patch

 drivers/of/of_pci.c    | 84 +++++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/of_pci.h |  2 ++
 2 files changed, 85 insertions(+), 1 deletion(-)

drivers/of/of_pci.c +2 -1

index 13e37e2..bcff301 100644
@@ -1,7 +1,8 @@
 #include <linux/kernel.h>
 #include <linux/export.h>
-#include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_pci.h>
+#include <linux/slab.h>
 #include <asm/prom.h>
 

drivers/of/of_pci.c struct device_node *of_pci_find_child_device(struct device_node *parent, · line 49

Quoted 11 lines

drivers/of/of_pci.c +6 -0

 static inline int __of_pci_pci_compare(struct device_node *node,

@@ -40,3 +41,84 @@ struct device_node *of_pci_find_child_device(struct device_node *parent,
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(of_pci_find_child_device);
+
+struct resource *of_pci_parse_ranges(struct device_node *node,
+				     unsigned int *countp)
+{
+	unsigned int count, i = 0;
+	struct resource *ranges;

I think res or pci_res would be clearer than ranges that this is a struct resource.

Quoted 11 lines · Rob Herring

Quoted 5 lines · Thierry Reding

This new function parses the ranges property of PCI device nodes into an array of struct resource elements. It is useful in multiple-port PCI host controller drivers to collect information about the ranges that it needs to forward to the respective ports.

It seems to me that some of the DT PCI code in arch/powerpc/kernel/pci* like pci_process_bridge_OF_ranges() should apply for ARM as well.

Quoted 2 lines

Each arch defining their own pci controller structs complicates this, but I would think at least the DT parsing can be common.

Yes, there's quite a lot of room for refactoring. When I first started work on this there had been some discussion about whether it would make sense to move PCI controller drivers into a common location to make it easier to refactor but the consensus at the time was that this should not be done.

arch/arm/boot/dts/tegra20.dtsi +13 -0

index a094c97..c886dff 100644
@@ -199,6 +199,68 @@
 		#size-cells = <0>;
 	};
 
+	pcie-controller {
+		compatible = "nvidia,tegra20-pcie";
+		reg = <0x80003000 0x00000800   /* PADS registers */
+		       0x80003800 0x00000200   /* AFI registers */
+		       0x81000000 0x01000000   /* configuration space */
+		       0x90000000 0x10000000>; /* extended configuration space */
+		interrupts = <0 98 0x04   /* controller interrupt */
+		              0 99 0x04>; /* MSI interrupt */
+		status = "disabled";
+
+		ranges = <0 0 0  0x80000000 0x00001000   /* root port 0 */
+			  0 1 0  0x81000000 0x00800000   /* port 0 config space */
+			  0 2 0  0x90000000 0x08000000   /* port 0 ext config space */

arch/arm/boot/dts/tegra20.dtsi +28 -0

+
+		#address-cells = <3>;
+		#size-cells = <1>;
+
+		pci at 0 {
+			device_type = "pciex";
+			reg = <0 0 0 0x1000>;
+			status = "disabled";
+
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			ranges = <0x80000000 0 0  0 1 0  0 0x00800000   /* config space */
+				  0x90000000 0 0  0 2 0  0 0x08000000   /* ext config space */
+				  0x81000000 0 0  0 3 0  0 0x00010000   /* I/O */
+				  0x82000000 0 0  0 4 0  0 0x08000000   /* non-prefetchable memory */
+				  0xc2000000 0 0  0 5 0  0 0x08000000>; /* prefetchable memory */
+
+			nvidia,num-lanes = <2>;
+		};
+
+		pci at 1 {
+			device_type = "pciex";
+			reg = <1 0 0 0x1000>;
+			status = "disabled";
+
+			#address-cells = <3>;
+			#size-cells = <2>;

Quoted 21 lines · Thierry Reding

arch/arm/boot/dts/tegra20.dtsi +13 -0

index a094c97..c886dff 100644
@@ -199,6 +199,68 @@
              #size-cells = <0>;
      };
 
+     pcie-controller {
+             compatible = "nvidia,tegra20-pcie";
+             reg = <0x80003000 0x00000800   /* PADS registers */
+                    0x80003800 0x00000200   /* AFI registers */
+                    0x81000000 0x01000000   /* configuration space */
+                    0x90000000 0x10000000>; /* extended configuration space */
+             interrupts = <0 98 0x04   /* controller interrupt */
+                           0 99 0x04>; /* MSI interrupt */
+             status = "disabled";
+
+             ranges = <0 0 0  0x80000000 0x00001000   /* root port 0 */
+                       0 1 0  0x81000000 0x00800000   /* port 0 config space */
+                       0 2 0  0x90000000 0x08000000   /* port 0 ext config space */

arch/arm/boot/dts/tegra20.dtsi line 224

Quoted 10 lines

arch/arm/boot/dts/tegra20.dtsi +10 -0

+                       0 3 0  0x82000000 0x00010000   /* port 0 downstream I/O */
+                       0 4 0  0xa0000000 0x08000000   /* port 0 non-prefetchable memory */
+                       0 5 0  0xb0000000 0x08000000   /* port 0 prefetchable memory */
+
+                       1 0 0  0x80001000 0x00001000   /* root port 1 */
+                       1 1 0  0x81800000 0x00800000   /* port 1 config space */
+                       1 2 0  0x98000000 0x08000000   /* port 1 ext config space */
+                       1 3 0  0x82010000 0x00010000   /* port 1 downstream I/O */
+                       1 4 0  0xa8000000 0x08000000   /* port 1 non-prefetchable memory */
+                       1 5 0  0xb8000000 0x08000000>; /* port 1 prefetchable memory */

I've been thinking about this some more. The translations for both the regular and extended configuration spaces are configured in the top- level PCIe controller. It is therefore wrong how they are passed to the PCI host bridges via the ranges property.

I remember Mitch saying that it should be passed down to the children because it is partitioned among them, but since the layout is compatible with ECAM, the partitioning isn't as simple as what's in the tree. In fact the partitions will be dependent on the number of devices attached to the host bridges.

Quoted 28 lines

arch/arm/boot/dts/tegra20.dtsi +28 -0

+
+             #address-cells = <3>;
+             #size-cells = <1>;
+
+             pci at 0 {
+                     device_type = "pciex";
+                     reg = <0 0 0 0x1000>;
+                     status = "disabled";
+
+                     #address-cells = <3>;
+                     #size-cells = <2>;
+
+                     ranges = <0x80000000 0 0  0 1 0  0 0x00800000   /* config space */
+                               0x90000000 0 0  0 2 0  0 0x08000000   /* ext config space */
+                               0x81000000 0 0  0 3 0  0 0x00010000   /* I/O */
+                               0x82000000 0 0  0 4 0  0 0x08000000   /* non-prefetchable memory */
+                               0xc2000000 0 0  0 5 0  0 0x08000000>; /* prefetchable memory */
+
+                     nvidia,num-lanes = <2>;
+             };
+
+             pci at 1 {
+                     device_type = "pciex";
+                     reg = <1 0 0 0x1000>;
+                     status = "disabled";
+
+                     #address-cells = <3>;
+                     #size-cells = <2>;

arch/arm/boot/dts/tegra20.dtsi line 262

Quoted 10 lines

arch/arm/boot/dts/tegra20.dtsi +10 -0

+
+                     ranges = <0x80000000 0 0  1 1 0  0 0x00800000   /* config space */
+                               0x90000000 0 0  1 2 0  0 0x08000000   /* ext config space */
+                               0x81000000 0 0  1 3 0  0 0x00010000   /* I/O */
+                               0x82000000 0 0  1 4 0  0 0x08000000   /* non-prefetchable memory */
+                               0xc2000000 0 0  1 5 0  0 0x08000000>; /* prefetchable memory */
+
+                     nvidia,num-lanes = <2>;
+             };
+     };

The same is true for the ranges properties of the PCI host bridge nodes. Which part of the configuration spaces maps to the children of the respective host bridge depends on the actual device hierarchy.

Would it be possible to alternatively pass the complete range to the children without further partitioning?

The driver doesn't actually care about the ranges property and only uses the values specified in the reg property of the pcie-controller node.

Thierry

Quoted 38 lines · Thierry Reding

Quoted 21 lines · Thierry Reding

arch/arm/boot/dts/tegra20.dtsi +13 -0

index a094c97..c886dff 100644
@@ -199,6 +199,68 @@
              #size-cells = <0>;
      };
 
+     pcie-controller {
+             compatible = "nvidia,tegra20-pcie";
+             reg = <0x80003000 0x00000800   /* PADS registers */
+                    0x80003800 0x00000200   /* AFI registers */
+                    0x81000000 0x01000000   /* configuration space */
+                    0x90000000 0x10000000>; /* extended configuration space */
+             interrupts = <0 98 0x04   /* controller interrupt */
+                           0 99 0x04>; /* MSI interrupt */
+             status = "disabled";
+
+             ranges = <0 0 0  0x80000000 0x00001000   /* root port 0 */
+                       0 1 0  0x81000000 0x00800000   /* port 0 config space */
+                       0 2 0  0x90000000 0x08000000   /* port 0 ext config space */

arch/arm/boot/dts/tegra20.dtsi line 224

Quoted 10 lines

arch/arm/boot/dts/tegra20.dtsi +10 -0

+                       0 3 0  0x82000000 0x00010000   /* port 0 downstream I/O */
+                       0 4 0  0xa0000000 0x08000000   /* port 0 non-prefetchable memory */
+                       0 5 0  0xb0000000 0x08000000   /* port 0 prefetchable memory */
+
+                       1 0 0  0x80001000 0x00001000   /* root port 1 */
+                       1 1 0  0x81800000 0x00800000   /* port 1 config space */
+                       1 2 0  0x98000000 0x08000000   /* port 1 ext config space */
+                       1 3 0  0x82010000 0x00010000   /* port 1 downstream I/O */
+                       1 4 0  0xa8000000 0x08000000   /* port 1 non-prefetchable memory */
+                       1 5 0  0xb8000000 0x08000000>; /* port 1 prefetchable memory */

I've been thinking about this some more. The translations for both the regular and extended configuration spaces are configured in the top- level PCIe controller. It is therefore wrong how they are passed to the PCI host bridges via the ranges property.

Quoted 5 lines

I remember Mitch saying that it should be passed down to the children because it is partitioned among them, but since the layout is compatible with ECAM, the partitioning isn't as simple as what's in the tree. In fact the partitions will be dependent on the number of devices attached to the host bridges.

I don't understand this last bit about the number of devices attached to the host bridges. Logically, the host bridge has a bus number aperture that you can know up front, even before you know anything about what devices are below it. On x86, for example, the ACPI _CRS method has something like "[bus 00-7f]" in it, which means that any buses in that range are below this bridge. That doesn't tell us anything about which buses actually have devices on them, of course; it's just analogous to the secondary and subordinate bus number registers in a P2P bridge.

Quoted 46 lines · Bjorn Helgaas

Quoted 38 lines · Thierry Reding

Quoted 21 lines · Thierry Reding

arch/arm/boot/dts/tegra20.dtsi +13 -0

index a094c97..c886dff 100644
@@ -199,6 +199,68 @@
              #size-cells = <0>;
      };
 
+     pcie-controller {
+             compatible = "nvidia,tegra20-pcie";
+             reg = <0x80003000 0x00000800   /* PADS registers */
+                    0x80003800 0x00000200   /* AFI registers */
+                    0x81000000 0x01000000   /* configuration space */
+                    0x90000000 0x10000000>; /* extended configuration space */
+             interrupts = <0 98 0x04   /* controller interrupt */
+                           0 99 0x04>; /* MSI interrupt */
+             status = "disabled";
+
+             ranges = <0 0 0  0x80000000 0x00001000   /* root port 0 */
+                       0 1 0  0x81000000 0x00800000   /* port 0 config space */
+                       0 2 0  0x90000000 0x08000000   /* port 0 ext config space */

arch/arm/boot/dts/tegra20.dtsi line 224

Quoted 10 lines

arch/arm/boot/dts/tegra20.dtsi +10 -0

+                       0 3 0  0x82000000 0x00010000   /* port 0 downstream I/O */
+                       0 4 0  0xa0000000 0x08000000   /* port 0 non-prefetchable memory */
+                       0 5 0  0xb0000000 0x08000000   /* port 0 prefetchable memory */
+
+                       1 0 0  0x80001000 0x00001000   /* root port 1 */
+                       1 1 0  0x81800000 0x00800000   /* port 1 config space */
+                       1 2 0  0x98000000 0x08000000   /* port 1 ext config space */
+                       1 3 0  0x82010000 0x00010000   /* port 1 downstream I/O */
+                       1 4 0  0xa8000000 0x08000000   /* port 1 non-prefetchable memory */
+                       1 5 0  0xb8000000 0x08000000>; /* port 1 prefetchable memory */

I've been thinking about this some more. The translations for both the regular and extended configuration spaces are configured in the top- level PCIe controller. It is therefore wrong how they are passed to the PCI host bridges via the ranges property.

Quoted 5 lines

I remember Mitch saying that it should be passed down to the children because it is partitioned among them, but since the layout is compatible with ECAM, the partitioning isn't as simple as what's in the tree. In fact the partitions will be dependent on the number of devices attached to the host bridges.

Quoted 9 lines

I don't understand this last bit about the number of devices attached to the host bridges. Logically, the host bridge has a bus number aperture that you can know up front, even before you know anything about what devices are below it. On x86, for example, the ACPI _CRS method has something like "[bus 00-7f]" in it, which means that any buses in that range are below this bridge. That doesn't tell us anything about which buses actually have devices on them, of course; it's just analogous to the secondary and subordinate bus number registers in a P2P bridge.

That's one of the issues I still need to take care of. Currently no bus resource is attached to the individual bridges (nor the PCI controller for that matter), so the PCI core will assign them dynamically.

If this
range is known at boot time we could assign ECAM ranges based on the bus numbers. Standard ECAM ranges, that is. On Tegra this won't work because as Stephen mentioned in a previous mail, the bus field is not the top field in the ECAM addresses. Basically what you have is this:

        [27:24] upper 4 bits of the register address for extended
                configuration space
        [23:16] bus number
        [15:11] device number
        [10: 8] device function
        [ 8: 0] register

So the ECAM space cannot be partitioned by bus number.

Quoted 57 lines · Thierry Reding

Quoted 46 lines · Bjorn Helgaas

Quoted 38 lines · Thierry Reding

Quoted 21 lines · Thierry Reding

arch/arm/boot/dts/tegra20.dtsi +13 -0

index a094c97..c886dff 100644
@@ -199,6 +199,68 @@
              #size-cells = <0>;
      };
 
+     pcie-controller {
+             compatible = "nvidia,tegra20-pcie";
+             reg = <0x80003000 0x00000800   /* PADS registers */
+                    0x80003800 0x00000200   /* AFI registers */
+                    0x81000000 0x01000000   /* configuration space */
+                    0x90000000 0x10000000>; /* extended configuration space */
+             interrupts = <0 98 0x04   /* controller interrupt */
+                           0 99 0x04>; /* MSI interrupt */
+             status = "disabled";
+
+             ranges = <0 0 0  0x80000000 0x00001000   /* root port 0 */
+                       0 1 0  0x81000000 0x00800000   /* port 0 config space */
+                       0 2 0  0x90000000 0x08000000   /* port 0 ext config space */

arch/arm/boot/dts/tegra20.dtsi line 224

Quoted 10 lines

arch/arm/boot/dts/tegra20.dtsi +10 -0

+                       0 3 0  0x82000000 0x00010000   /* port 0 downstream I/O */
+                       0 4 0  0xa0000000 0x08000000   /* port 0 non-prefetchable memory */
+                       0 5 0  0xb0000000 0x08000000   /* port 0 prefetchable memory */
+
+                       1 0 0  0x80001000 0x00001000   /* root port 1 */
+                       1 1 0  0x81800000 0x00800000   /* port 1 config space */
+                       1 2 0  0x98000000 0x08000000   /* port 1 ext config space */
+                       1 3 0  0x82010000 0x00010000   /* port 1 downstream I/O */
+                       1 4 0  0xa8000000 0x08000000   /* port 1 non-prefetchable memory */
+                       1 5 0  0xb8000000 0x08000000>; /* port 1 prefetchable memory */

I've been thinking about this some more. The translations for both the regular and extended configuration spaces are configured in the top- level PCIe controller. It is therefore wrong how they are passed to the PCI host bridges via the ranges property.

Quoted 5 lines

I remember Mitch saying that it should be passed down to the children because it is partitioned among them, but since the layout is compatible with ECAM, the partitioning isn't as simple as what's in the tree. In fact the partitions will be dependent on the number of devices attached to the host bridges.

Quoted 9 lines

I don't understand this last bit about the number of devices attached to the host bridges. Logically, the host bridge has a bus number aperture that you can know up front, even before you know anything about what devices are below it. On x86, for example, the ACPI _CRS method has something like "[bus 00-7f]" in it, which means that any buses in that range are below this bridge. That doesn't tell us anything about which buses actually have devices on them, of course; it's just analogous to the secondary and subordinate bus number registers in a P2P bridge.

Quoted 3 lines

That's one of the issues I still need to take care of. Currently no bus resource is attached to the individual bridges (nor the PCI controller for that matter), so the PCI core will assign them dynamically.

So your PCI controller driver knows how to program the controller bus number aperture? Sometimes people start by assuming that two host bridges both have [bus 00-ff] apertures, then they enumerate below the first and adjust the bus number apertures based on what they found. For example, if they found buses 00-12 behind the first bridge, they make the apertures [bus 00-12] for the first bridge and [bus 13-ff] for the second. That might be the case, depending on what firmware set up, but it seems like a dubious way to do it, and of course it precludes a lot of hot-plug scenarios.

Quoted 63 lines · Bjorn Helgaas

Quoted 57 lines · Thierry Reding

Quoted 46 lines · Bjorn Helgaas

Quoted 38 lines · Thierry Reding

Quoted 21 lines · Thierry Reding

arch/arm/boot/dts/tegra20.dtsi +13 -0

index a094c97..c886dff 100644
@@ -199,6 +199,68 @@
              #size-cells = <0>;
      };
 
+     pcie-controller {
+             compatible = "nvidia,tegra20-pcie";
+             reg = <0x80003000 0x00000800   /* PADS registers */
+                    0x80003800 0x00000200   /* AFI registers */
+                    0x81000000 0x01000000   /* configuration space */
+                    0x90000000 0x10000000>; /* extended configuration space */
+             interrupts = <0 98 0x04   /* controller interrupt */
+                           0 99 0x04>; /* MSI interrupt */
+             status = "disabled";
+
+             ranges = <0 0 0  0x80000000 0x00001000   /* root port 0 */
+                       0 1 0  0x81000000 0x00800000   /* port 0 config space */
+                       0 2 0  0x90000000 0x08000000   /* port 0 ext config space */

arch/arm/boot/dts/tegra20.dtsi line 224

Quoted 10 lines

arch/arm/boot/dts/tegra20.dtsi +10 -0

+                       0 3 0  0x82000000 0x00010000   /* port 0 downstream I/O */
+                       0 4 0  0xa0000000 0x08000000   /* port 0 non-prefetchable memory */
+                       0 5 0  0xb0000000 0x08000000   /* port 0 prefetchable memory */
+
+                       1 0 0  0x80001000 0x00001000   /* root port 1 */
+                       1 1 0  0x81800000 0x00800000   /* port 1 config space */
+                       1 2 0  0x98000000 0x08000000   /* port 1 ext config space */
+                       1 3 0  0x82010000 0x00010000   /* port 1 downstream I/O */
+                       1 4 0  0xa8000000 0x08000000   /* port 1 non-prefetchable memory */
+                       1 5 0  0xb8000000 0x08000000>; /* port 1 prefetchable memory */

I've been thinking about this some more. The translations for both the regular and extended configuration spaces are configured in the top- level PCIe controller. It is therefore wrong how they are passed to the PCI host bridges via the ranges property.

Quoted 5 lines

I remember Mitch saying that it should be passed down to the children because it is partitioned among them, but since the layout is compatible with ECAM, the partitioning isn't as simple as what's in the tree. In fact the partitions will be dependent on the number of devices attached to the host bridges.

Quoted 9 lines

I don't understand this last bit about the number of devices attached to the host bridges. Logically, the host bridge has a bus number aperture that you can know up front, even before you know anything about what devices are below it. On x86, for example, the ACPI _CRS method has something like "[bus 00-7f]" in it, which means that any buses in that range are below this bridge. That doesn't tell us anything about which buses actually have devices on them, of course; it's just analogous to the secondary and subordinate bus number registers in a P2P bridge.

Quoted 3 lines

That's one of the issues I still need to take care of. Currently no bus resource is attached to the individual bridges (nor the PCI controller for that matter), so the PCI core will assign them dynamically.

Quoted 9 lines

So your PCI controller driver knows how to program the controller bus number aperture? Sometimes people start by assuming that two host bridges both have [bus 00-ff] apertures, then they enumerate below the first and adjust the bus number apertures based on what they found. For example, if they found buses 00-12 behind the first bridge, they make the apertures [bus 00-12] for the first bridge and [bus 13-ff] for the second. That might be the case, depending on what firmware set up, but it seems like a dubious way to do it, and of course it precludes a lot of hot-plug scenarios.

No, that's not what I meant. What happens is that no pre-assigned bus range is specified for either of the host bridges, so that the range 0x00-0xff will be assigned by default in pci_scan_root_bus().

Quoted 74 lines · Thierry Reding

Quoted 63 lines · Bjorn Helgaas

Quoted 57 lines · Thierry Reding

Quoted 46 lines · Bjorn Helgaas

Quoted 38 lines · Thierry Reding

Quoted 21 lines · Thierry Reding

arch/arm/boot/dts/tegra20.dtsi +13 -0

index a094c97..c886dff 100644
@@ -199,6 +199,68 @@
              #size-cells = <0>;
      };
 
+     pcie-controller {
+             compatible = "nvidia,tegra20-pcie";
+             reg = <0x80003000 0x00000800   /* PADS registers */
+                    0x80003800 0x00000200   /* AFI registers */
+                    0x81000000 0x01000000   /* configuration space */
+                    0x90000000 0x10000000>; /* extended configuration space */
+             interrupts = <0 98 0x04   /* controller interrupt */
+                           0 99 0x04>; /* MSI interrupt */
+             status = "disabled";
+
+             ranges = <0 0 0  0x80000000 0x00001000   /* root port 0 */
+                       0 1 0  0x81000000 0x00800000   /* port 0 config space */
+                       0 2 0  0x90000000 0x08000000   /* port 0 ext config space */

arch/arm/boot/dts/tegra20.dtsi line 224

Quoted 10 lines

arch/arm/boot/dts/tegra20.dtsi +10 -0

+                       0 3 0  0x82000000 0x00010000   /* port 0 downstream I/O */
+                       0 4 0  0xa0000000 0x08000000   /* port 0 non-prefetchable memory */
+                       0 5 0  0xb0000000 0x08000000   /* port 0 prefetchable memory */
+
+                       1 0 0  0x80001000 0x00001000   /* root port 1 */
+                       1 1 0  0x81800000 0x00800000   /* port 1 config space */
+                       1 2 0  0x98000000 0x08000000   /* port 1 ext config space */
+                       1 3 0  0x82010000 0x00010000   /* port 1 downstream I/O */
+                       1 4 0  0xa8000000 0x08000000   /* port 1 non-prefetchable memory */
+                       1 5 0  0xb8000000 0x08000000>; /* port 1 prefetchable memory */

I've been thinking about this some more. The translations for both the regular and extended configuration spaces are configured in the top- level PCIe controller. It is therefore wrong how they are passed to the PCI host bridges via the ranges property.

Quoted 5 lines

I remember Mitch saying that it should be passed down to the children because it is partitioned among them, but since the layout is compatible with ECAM, the partitioning isn't as simple as what's in the tree. In fact the partitions will be dependent on the number of devices attached to the host bridges.

Quoted 9 lines

I don't understand this last bit about the number of devices attached to the host bridges. Logically, the host bridge has a bus number aperture that you can know up front, even before you know anything about what devices are below it. On x86, for example, the ACPI _CRS method has something like "[bus 00-7f]" in it, which means that any buses in that range are below this bridge. That doesn't tell us anything about which buses actually have devices on them, of course; it's just analogous to the secondary and subordinate bus number registers in a P2P bridge.

Quoted 3 lines

That's one of the issues I still need to take care of. Currently no bus resource is attached to the individual bridges (nor the PCI controller for that matter), so the PCI core will assign them dynamically.

Quoted 9 lines

So your PCI controller driver knows how to program the controller bus number aperture? Sometimes people start by assuming that two host bridges both have [bus 00-ff] apertures, then they enumerate below the first and adjust the bus number apertures based on what they found. For example, if they found buses 00-12 behind the first bridge, they make the apertures [bus 00-12] for the first bridge and [bus 13-ff] for the second. That might be the case, depending on what firmware set up, but it seems like a dubious way to do it, and of course it precludes a lot of hot-plug scenarios.

Quoted 3 lines

No, that's not what I meant. What happens is that no pre-assigned bus range is specified for either of the host bridges, so that the range 0x00-0xff will be assigned by default in pci_scan_root_bus().

My concern is about making the kernel's idea of the host bridge bus number aperture match what the hardware is doing. I'm pretty sure that the default [bus 00-ff] range assigned by pci_scan_root_bus() doesn't actually match the hardware in most cases, at least when we have multiple host bridges in the same PCI domain.

For example, if you don't supply a bus number range, pci_scan_root_bus() will assume [bus 00-ff] for both host bridges. But if you could put an analyzer on each of the root buses and then read bus 0 config space, will you see that config transaction on *both* buses? I doubt it.

You have to know at least the bus number of the root bus up front before you can even start enumerating it. The only way to learn that is by reading registers in the host bridge or by some external mechanism like ACPI or device tree. That's the beginning of the bus number aperture. The end of the aperture is similar: we can't reliably determine it by enumerating devices below the host bridge, so we have to know it up front. You can enumerate starting with the root bus number and assigning new subordinate bus numbers as necessary, but unless you know the host bridge aperture to begin with, you could inadvertently assign a new bus number that actually belongs to a different host bridge.

Yes, that was my understanding as well. So currently I haven't seen any problems with this because I only use one of the two host bridges. But I suppose I should add code to initialize the bus number aperture properly either via platform device resources (for the non-DT case) and the device tree otherwise.

Quoted 5 lines · Thierry Reding

Yes, that was my understanding as well. So currently I haven't seen any problems with this because I only use one of the two host bridges. But I suppose I should add code to initialize the bus number aperture properly either via platform device resources (for the non-DT case) and the device tree otherwise.

I think when we last discussed this, the assumption was that each root port has its own config space range and its own pci domain, so you don't have to worry about bus apertures because each root port can then have all 255 bus numbers. Has that turned out to be incorrect now?

Quoted 5 lines · Thierry Reding

Yes, that was my understanding as well. So currently I haven't seen any problems with this because I only use one of the two host bridges. But I suppose I should add code to initialize the bus number aperture properly either via platform device resources (for the non-DT case) and the device tree otherwise.

I think when we last discussed this, the assumption was that each root port has its own config space range and its own pci domain, so you don't have to worry about bus apertures because each root port can then have all 255 bus numbers. Has that turned out to be incorrect now?

At least for the config space this is incorrect. There's a single region to access the configuration space for all devices below the PCIe controller. So it is shared by both (Tegra20) or all three (Tegra30) root ports.

I'm not sure about PCI domains. Do you have any good pointers as to where I could read up on them? If they need special hardware support, then I think Tegra doesn't support them either. At least I haven't come across any mention of domains while going through the, admittedly some- what sparse on PCIe, Tegra documentation.

Quoted 10 lines · Thierry Reding

At least for the config space this is incorrect. There's a single region to access the configuration space for all devices below the PCIe controller. So it is shared by both (Tegra20) or all three (Tegra30) root ports.

I'm not sure about PCI domains. Do you have any good pointers as to where I could read up on them? If they need special hardware support, then I think Tegra doesn't support them either. At least I haven't come across any mention of domains while going through the, admittedly some- what sparse on PCIe, Tegra documentation.

I was referring to the same thing here: if you had a separate config space for each root port, they would by definition be separate PCI domain. So you don't have them.

Quoted 57 lines · Thierry Reding

Quoted 46 lines · Bjorn Helgaas

Quoted 38 lines · Thierry Reding

Quoted 21 lines · Thierry Reding

arch/arm/boot/dts/tegra20.dtsi +13 -0

index a094c97..c886dff 100644
@@ -199,6 +199,68 @@
              #size-cells = <0>;
      };
 
+     pcie-controller {
+             compatible = "nvidia,tegra20-pcie";
+             reg = <0x80003000 0x00000800   /* PADS registers */
+                    0x80003800 0x00000200   /* AFI registers */
+                    0x81000000 0x01000000   /* configuration space */
+                    0x90000000 0x10000000>; /* extended configuration space */
+             interrupts = <0 98 0x04   /* controller interrupt */
+                           0 99 0x04>; /* MSI interrupt */
+             status = "disabled";
+
+             ranges = <0 0 0  0x80000000 0x00001000   /* root port 0 */
+                       0 1 0  0x81000000 0x00800000   /* port 0 config space */
+                       0 2 0  0x90000000 0x08000000   /* port 0 ext config space */

arch/arm/boot/dts/tegra20.dtsi line 224

Quoted 10 lines

arch/arm/boot/dts/tegra20.dtsi +10 -0

+                       0 3 0  0x82000000 0x00010000   /* port 0 downstream I/O */
+                       0 4 0  0xa0000000 0x08000000   /* port 0 non-prefetchable memory */
+                       0 5 0  0xb0000000 0x08000000   /* port 0 prefetchable memory */
+
+                       1 0 0  0x80001000 0x00001000   /* root port 1 */
+                       1 1 0  0x81800000 0x00800000   /* port 1 config space */
+                       1 2 0  0x98000000 0x08000000   /* port 1 ext config space */
+                       1 3 0  0x82010000 0x00010000   /* port 1 downstream I/O */
+                       1 4 0  0xa8000000 0x08000000   /* port 1 non-prefetchable memory */
+                       1 5 0  0xb8000000 0x08000000>; /* port 1 prefetchable memory */

I've been thinking about this some more. The translations for both the regular and extended configuration spaces are configured in the top- level PCIe controller. It is therefore wrong how they are passed to the PCI host bridges via the ranges property.

Quoted 5 lines

I remember Mitch saying that it should be passed down to the children because it is partitioned among them, but since the layout is compatible with ECAM, the partitioning isn't as simple as what's in the tree. In fact the partitions will be dependent on the number of devices attached to the host bridges.

Quoted 9 lines

I don't understand this last bit about the number of devices attached to the host bridges. Logically, the host bridge has a bus number aperture that you can know up front, even before you know anything about what devices are below it. On x86, for example, the ACPI _CRS method has something like "[bus 00-7f]" in it, which means that any buses in that range are below this bridge. That doesn't tell us anything about which buses actually have devices on them, of course; it's just analogous to the secondary and subordinate bus number registers in a P2P bridge.

Quoted 3 lines

That's one of the issues I still need to take care of. Currently no bus resource is attached to the individual bridges (nor the PCI controller for that matter), so the PCI core will assign them dynamically.

So your PCI controller driver knows how to program the controller bus number aperture? Sometimes people start by assuming that two host bridges both have [bus 00-ff] apertures, then they enumerate below the first and adjust the bus number apertures based on what they found. For example, if they found buses 00-12 behind the first bridge, they make the apertures [bus 00-12] for the first bridge and [bus 13-ff] for the second. That might be the case, depending on what firmware set up, but it seems like a dubious way to do it, and of course it precludes a lot of hot-plug scenarios.

Quoted 13 lines

If this
range is known at boot time we could assign ECAM ranges based on the bus numbers. Standard ECAM ranges, that is. On Tegra this won't work because as Stephen mentioned in a previous mail, the bus field is not the top field in the ECAM addresses. Basically what you have is this:

        [27:24] upper 4 bits of the register address for extended
                configuration space
        [23:16] bus number
        [15:11] device number
        [10: 8] device function
        [ 8: 0] register

Quoted 1 line

So the ECAM space cannot be partitioned by bus number.

Ah, OK, so definitely not standard PCIe ECAM.

This patch series adds support for device tree based probing of the PCIe controller found on Tegra SoCs.

Quoted 2 lines · Thierry Reding

This patch series adds support for device tree based probing of the PCIe controller found on Tegra SoCs.

Thierry,

I think one thing that would help here would be to split up this series into one per subsystem, and to get all the dependencies merged by the respective maintainers. Preferably, each subsystem would export a stable branch (perhaps consisting of just these patches) that I can then merge into the Tegra tree and use as a basis for the PCIe driver itself. Hopefully this approach will get more traction on all the non-Tegra changes. Does that sound like a good plan?

Or, I can just take it all through the Tegra tree, but I'll definitely need acks on all/most of the non-Tegra patches, since although the patches themselves are quite small, I'd like to be sure that maintainers are OK with the conceptual changes, like initializing PCI controllers later than at present etc.

Quoted 6 lines · Stephen Warren

Quoted 2 lines · Thierry Reding

This patch series adds support for device tree based probing of the PCIe controller found on Tegra SoCs.

Thierry,

Quoted 8 lines

I think one thing that would help here would be to split up this series into one per subsystem, and to get all the dependencies merged by the respective maintainers. Preferably, each subsystem would export a stable branch (perhaps consisting of just these patches) that I can then merge into the Tegra tree and use as a basis for the PCIe driver itself. Hopefully this approach will get more traction on all the non-Tegra changes. Does that sound like a good plan?

I don't understand. The series is already split up into per-subsystem patches. I just didn't want to post them separately so everybody on Cc would be able to see the big picture and the reason why the patch was required.

Quoted 16 lines · Thierry Reding

Quoted 6 lines · Stephen Warren

Quoted 2 lines · Thierry Reding

This patch series adds support for device tree based probing of the PCIe controller found on Tegra SoCs.

Thierry,

Quoted 8 lines

I think one thing that would help here would be to split up this series into one per subsystem, and to get all the dependencies merged by the respective maintainers. Preferably, each subsystem would export a stable branch (perhaps consisting of just these patches) that I can then merge into the Tegra tree and use as a basis for the PCIe driver itself. Hopefully this approach will get more traction on all the non-Tegra changes. Does that sound like a good plan?

Quoted 4 lines

I don't understand. The series is already split up into per-subsystem patches. I just didn't want to post them separately so everybody on Cc would be able to see the big picture and the reason why the patch was required.

There are separate patches that touch the different subsystems, but they're all scattered throughout the one series, rather than having e.g.:

* a series for the PCI tree
* a series for the ARM core tree
* a series for the OF/DT tree
* a series for the Tegra tree, which indicates it depends on all of those.

I'm wondering if the reason you haven't seen much discussion/Acks from the maintainers of the non-Tegra trees is because this is a big scary series that touches a lot of stuff, and it's not necessarily clear for people unfamiliar with it why they're being CCd on it.

This patch series adds support for device tree based probing of the PCIe controller found on Tegra SoCs.

	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));

Quoted 2 lines · Thierry Reding

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():

Quoted 5 lines

	for (idx = 0; idx < 6; idx++) {
		/* Only set up the requested stuff */
		if (!(mask & (1 << idx)))
			continue;

Quoted 4 lines

		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?

Quoted 2 lines · Thierry Reding

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():

Quoted 9 lines

	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.

From ebd69ae0a3d076e574da74d963cb3834b71dc6ad Mon Sep 17 00:00:00 2001

Quoted 8 lines · Stephen Warren

Quoted 2 lines · Thierry Reding

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?

Quoted 2 lines

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.

[    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

[    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

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

[    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

[    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]

[    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]

[    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]

[    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

[    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]

Quoted 9 lines · Thierry Reding

Quoted 8 lines · Stephen Warren

Quoted 2 lines · Thierry Reding

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?

...

Quoted 2 lines

Quoted 2 lines

PCI: Device 0000:01:00.0 not available because of resource collisions

...

Quoted 7 lines

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:

Quoted 3 lines

[    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:

Quoted 3 lines

[    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:

Quoted 5 lines

		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:

Quoted 4 lines

[    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:

Quoted 4 lines

[    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]

...

Quoted 4 lines

[    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:)

Quoted 3 lines

[    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):

Quoted 4 lines

[    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)

Quoted 4 lines

[    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)

Quoted 4 lines

[    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.

Quoted 31 lines · Stephen Warren

Quoted 9 lines · Thierry Reding

Quoted 8 lines · Stephen Warren

Quoted 2 lines · Thierry Reding

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?

...

Quoted 2 lines

Quoted 2 lines

PCI: Device 0000:01:00.0 not available because of resource collisions

...

Quoted 7 lines

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:

Quoted 3 lines

[    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

Quoted 14 lines

Were replaced with the following with your patch:

Quoted 3 lines

[    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:

Quoted 5 lines

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

Looking at the code some more, this may be caused by the pci_remap_io() patch series, so you might want to revert that patch and see if it fixes the I/O resources.

Quoted 52 lines

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

Quoted 4 lines

[    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:

Quoted 4 lines

[    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]

...

Quoted 4 lines

[    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:)

Quoted 3 lines

[    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):

Quoted 4 lines

[    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)

Quoted 4 lines

[    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)

Quoted 4 lines

[    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.

Quoted 2 lines

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.

There's the io_offset and mem_offset fields that I've completely ignored up to now. Can you try the patch below to see if it changes anything? I'm sorry but I can't test any of this myself right now.

Thierry

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Quoted 47 lines · Thierry Reding

Quoted 45 lines · Stephen Warren

Quoted 9 lines · Thierry Reding

Quoted 8 lines · Stephen Warren

Quoted 2 lines · Thierry Reding

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?

...

Quoted 2 lines

Quoted 2 lines

PCI: Device 0000:01:00.0 not available because of resource collisions

...

Quoted 7 lines

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:

Quoted 3 lines

[    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:

Quoted 3 lines

[    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:

Quoted 5 lines

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

Quoted 4 lines

Looking at the code some more, this may be caused by the pci_remap_io() patch series, so you might want to revert that patch and see if it fixes the I/O resources.

Humm... But this patch deals with the i/o space and it is failing below on the memory space.

Quoted 53 lines

Quoted 53 lines

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

Quoted 4 lines

[    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:

Quoted 4 lines

[    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]

...

Quoted 4 lines

[    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:)

Quoted 3 lines

[    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):

Quoted 4 lines

[    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)

Quoted 4 lines

[    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)

Quoted 4 lines

[    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.

Quoted 6 lines

Quoted 2 lines

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.

There's the io_offset and mem_offset fields that I've completely ignored up to now. Can you try the patch below to see if it changes anything? I'm sorry but I can't test any of this myself right now.

Arnd and I discussed io_offset some. I don't think either of us can figure out when it should be anything but 0 at least if pci i/o bus addresses start at 0.

I don't think mem_offset is the issue. I think perhaps you need to set pcibios_min_mem to the memory window base (0xa0000000), but that's just a guess.

Quoted 9 lines · Thierry Reding

Quoted 8 lines · Stephen Warren

Quoted 2 lines · Thierry Reding

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?

...

Quoted 2 lines

Quoted 2 lines

PCI: Device 0000:01:00.0 not available because of resource collisions

...

Quoted 6 lines

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.

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

Quoted 3 lines

[    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:

Quoted 3 lines

[    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:

Quoted 5 lines

              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:

Quoted 4 lines

[    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:

Quoted 4 lines

[    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]

...

Quoted 4 lines

[    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:)

Quoted 3 lines

[    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):

Quoted 4 lines

[    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)

Quoted 4 lines

[    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)

Quoted 4 lines

[    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.

Quoted 13 lines · Stephen Warren

Quoted 9 lines · Thierry Reding

Quoted 8 lines · Stephen Warren

Quoted 2 lines · Thierry Reding

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?

...

Quoted 2 lines

Quoted 2 lines

PCI: Device 0000:01:00.0 not available because of resource collisions

Quoted 7 lines

...

Quoted 6 lines

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.

Quoted 25 lines

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

Quoted 3 lines

[    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:

Quoted 3 lines

[    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:

Quoted 5 lines

              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:

Quoted 8 lines

Quoted 4 lines

[    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.

Quoted 34 lines

Boot log using board files:

Quoted 4 lines

[    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]

...

Quoted 4 lines

[    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:)

Quoted 3 lines

[    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):

Quoted 4 lines

[    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)

Quoted 4 lines

[    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)

Quoted 4 lines

[    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]

Quoted 5 lines

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.

On Mon, Aug 13, 2012 at 11:47 AM, Stephen Warren <[email protected]> wrote:

Quoted 18 lines

whereas for a device tree boot:

(same):

Quoted 4 lines

[    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)

Quoted 4 lines

[    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)

Quoted 4 lines

[    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]

Quoted 5 lines

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.

Quoted 1 line · Bjorn Helgaas

On Mon, Aug 13, 2012 at 11:47 AM, Stephen Warren <[email protected]> wrote:

...

Quoted 24 lines

Quoted 18 lines

whereas for a device tree boot:

(same):

Quoted 4 lines

[    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)

Quoted 4 lines

[    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)

Quoted 4 lines

[    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]

Quoted 5 lines

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.

Quoted 4 lines

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?

Quoted 47 lines · Stephen Warren

Quoted 1 line · Bjorn Helgaas

On Mon, Aug 13, 2012 at 11:47 AM, Stephen Warren <[email protected]> wrote:

...

Quoted 24 lines

Quoted 18 lines

whereas for a device tree boot:

(same):

Quoted 4 lines

[    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)

Quoted 4 lines

[    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)

Quoted 4 lines

[    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]

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 48 lines · Thierry Reding

Quoted 47 lines · Stephen Warren

Quoted 1 line · Bjorn Helgaas

On Mon, Aug 13, 2012 at 11:47 AM, Stephen Warren <[email protected]> wrote:

...

Quoted 24 lines

Quoted 18 lines

whereas for a device tree boot:

(same):

Quoted 4 lines

[    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)

Quoted 4 lines

[    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)

Quoted 4 lines

[    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]

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 64 lines · Bjorn Helgaas

Quoted 48 lines · Thierry Reding

Quoted 47 lines · Stephen Warren

Quoted 1 line · Bjorn Helgaas

On Mon, Aug 13, 2012 at 11:47 AM, Stephen Warren <[email protected]> wrote:

...

Quoted 24 lines

Quoted 18 lines

whereas for a device tree boot:

(same):

Quoted 4 lines

[    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)

Quoted 4 lines

[    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)

Quoted 4 lines

[    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]

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 6 lines

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()

Quoted 1 line

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:

On 08/14/2012 04:58 PM, Stephen Warren wrote:

Quoted 2 lines

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.

Quoted 1 line · Stephen Warren

On 08/14/2012 04:58 PM, Stephen Warren wrote:

...

Quoted 8 lines

Quoted 2 lines

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:-(

Quoted 1 line

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...

Quoted 11 lines · Stephen Warren

Quoted 1 line · Stephen Warren

On 08/14/2012 04:58 PM, Stephen Warren wrote:

...

Quoted 8 lines

Quoted 2 lines

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:-(

Quoted 8 lines

Quoted 1 line

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.

Quoted 7 lines

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? =)

Quoted 31 lines · Thierry Reding

Quoted 11 lines · Stephen Warren

Quoted 1 line · Stephen Warren

On 08/14/2012 04:58 PM, Stephen Warren wrote:

...

Quoted 8 lines

Quoted 2 lines

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:-(

Quoted 8 lines

Quoted 1 line

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.

Quoted 7 lines

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...

Quoted 3 lines

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.

Quoted 70 lines · Stephen Warren

Quoted 57 lines · Bjorn Helgaas

Quoted 48 lines · Thierry Reding

Quoted 47 lines · Stephen Warren

Quoted 1 line · Bjorn Helgaas

On Mon, Aug 13, 2012 at 11:47 AM, Stephen Warren <[email protected]> wrote:

...

Quoted 24 lines

Quoted 18 lines

whereas for a device tree boot:

(same):

Quoted 4 lines

[    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)

Quoted 4 lines

[    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)

Quoted 4 lines

[    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]

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 6 lines

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?

Quoted 8 lines

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.

Quoted 2 lines

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 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?

Quoted 5 lines · Stephen Warren

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?