PCIe support for the Armada 370 and Armada XP SoCs

Hello,

This series of patches introduces PCIe support for the Marvell Armada 370 and Armada XP. Even though the code is working, this first posting is done as a RFC, as there are a number of remaining questions to solve.

First, a quick description of the patches:

* Patch 1 fixes a problem in lib/devres.c that prevents certain PCI-related functions from being visible on NO_IOPORT platforms.

* Patch 2 slightly improves the Armada 370 gatable clock driver as a preparation for the PCIe introduction.

* Patch 3 and 4 are cleanup/refactoring of the common plat-orion address decoding code, in preparation for further changes related to PCIe.

* Patch 5 introduces in the common plat-orion address decoding code functions to allocate/free an address decoding window. Until now, the address decoding windows were configured statically. With Armada XP having up to 10 PCIe interfaces, we don't want to allocate useless address decoding windows statically, so we move to a more dynamic model in which address decoding windows are configured only for the PCIe interfaces that are actually in use.

* Patch 6 improves the Armada 370/XP specific address decoding code to provide functions that add and remove an address decoding window for a given PCIe interface. It relies on the common functions added in patch 5.

* Patch 7 makes the common plat-orion PCIe code available on PLAT_ORION platforms such as ARCH_MVEBU.

* Patch 8 contains the Armada 370/XP PCIe driver itself, that implements the necessary operations required by the ARM PCI core, and configures the address decoding windows as needed. This driver relies on a Device Tree description of the PCIe interfaces.

* Patch 9 marks the ARCH_MVEBU platform has having PCI available, which allows the compilation of the PCIe support.

* Patches 10 and 11 add the SoC-level Device Tree informations related to PCIe for Armada 370 and Armada XP.

* Patch 12, 13, 14 and 15 add the board-level Device Tree informations related to PCIe for the Armada XP DB, Armada 370 DB, PlatHome OpenBlocks AX3-4 and GlobalScale Mirabox boards.

* Patch 16 updates mvebu_defconfig with PCI and USB support.

In the pending issues:

* The most annoying problem is that when the hw_pci->setup() operation is called, we don't know the size of the memory and I/O regions that each PCI device will need. And on Marvell SoCs, for each PCI device, we have to set up an address decoding window that associates a portion of the physical address space with a given device. For now, we allocate 64 MB for each of those address decoding windows that point to PCIe memory regions, even if many PCIe devices need only a few KBs of memory regions. This over-allocation is an issue as we may have up to 10 PCIe interfaces on Armada XP, therefore consuming up to 640 MB of physical address space. And still, the 64 MB choice we have made may be too small for some PCIe devices (but is way too large for many of them).

Therefore, we would need a way of knowing the size of the different BARs for each PCI device, in order to appropriately configure our address decoding windows. Suggestions on how to achieve that are welcome.

* Contrary to the PCIe support for older Marvell SoCs, this one is a regular platform driver. Therefore, should it be stored in some other directory in drivers/pci/ ? If so, how do we handle the fact that our PCIe driver needs to call address decoding functions that will continue to be exposed through arch/arm/mach-mvebu/ specific code and headers ?

* The PCIe driver is currently initialized at the subsys_initcall() level, and not the more classical module_init() level. The reason is that module_init() initialization is too late, and the PCI quirks code tries to access PCI devices before our PCI driver had the chance to create the address decoding windows that are mandatory to access the PCIe devices on Marvell SoCs. Is this a problem?

* Is the Device Tree binding OK ? Compared to other Device Tree bindings for PCI controllers in the PowerPC world, we have a lot less addresses hardcoded in the Device Tree, as those addresses are allocated dynamically at runtime.

My TODO-list also contains:

* If the driver gets moved to some common place like drivers/pci/, use it for mach-kirkwood, mach-mv78xx0 and mach-dove, and maybe mach-orion5x.

* Figure out how all PCIe interfaces are used on the PlatHome OpenBricks AX3-4 platform. For now, only the one corresponding to the internal mini-PCIe slot is supported, but the japanese technical documentation seems to mention an USB controller on the PCIe bus. I've already asked the manufacturer more details about this.

* Probably a lot more things after this first round of reviews :-)

This patch set applies on top of arm-soc/for-next, and has been pushed at:

git://github.com/MISL-EBU-System-SW/mainline-public.git marvell-pcie-v1

Thanks,

Thomas

The pcim_*() functions are used by the libata-sff subsystem, and this subsystem is used for many SATA drivers on ARM platforms that do not necessarily have I/O ports.

---
 lib/devres.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 80b9c76..5639c3e 100644
@@ -195,6 +195,7 @@ void devm_ioport_unmap(struct device *dev, void __iomem *addr)
 			       devm_ioport_map_match, (void *)addr));
 }
 EXPORT_SYMBOL(devm_ioport_unmap);
+#endif /* CONFIG_HAS_IOPORT */
 
 #ifdef CONFIG_PCI
 /*

@@ -400,4 +401,3 @@ void pcim_iounmap_regions(struct pci_dev *pdev, int mask)
 }
 EXPORT_SYMBOL(pcim_iounmap_regions);
 #endif /* CONFIG_PCI */
-#endif /* CONFIG_HAS_IOPORT */

1.7.9.5

I think this one is wrong as the CONFIG_HAS_IOPORT does not refer to the presence of PIO ports but to whether or not they provide an ioport_map function. If there is no ioport_map(), devm_pci_iomap will fail to link as far as I can tell.

Arnd

Dear Arnd Bergmann,

The problem is that on ARCH_MULTI_V7, ARCH_VEXPRESS is forcefully enabled. And ARCH_VEXPRESS selects NO_IOPORT.. so you don't have the pcim_*() functions, and therefore libata-sff.c (needed for many SATA drivers) will not build. How do you solve this?

I'm not sure which devm_pci_iomap() you're referring to since my patch makes only the pcim_iomap_table(), pcim_iomap(), pcim_iounmap(), pcim_iomap_regions(), pcim_iomap_regions_request_all() and pcim_iounmap_regions() available under CONFIG_PCI instead of CONFIG_PCI && CONFIG_HAS_IOPORT.

So maybe you were referring to pcim_iomap(). I haven't checked in details, but I guess it builds because ioport_map() is implemented in arch/arm/mm/iomap.c.

Best regards,

Thomas

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

What you describe here are probable two bugs, and we should fix both:

* ARCH_VEXPRESS should not select NO_IOPORT. It's generally wrong to select this in combination with ARCH_MULTIPLATFORM, when some of the other platforms you may enable actually have IOPORT mapping support.

* We should not unconditionally select ARCH_VEXPRESS from ARCH_MULTIPLATFORM. There is no reason why we would enable that platform for building a kernel that runs on only one other platform.

Sorry, I meant pcim_iomap.

Right. If an ioport_map function is provided for a given platform, we should also set HAVE_IOPORT and vice versa. This is probably fallout of the io.h conversion for multiplatform.

Arnd

No. ARCH_VEXPRESS selects NO_IOPORT because it _does_ _not_ support PCI/ISA IO space. That in itself is reasonable, but what isn't reasonable is the _negative_ logic being used. Negative logic in the config system always tends to provoke this kind of sillyness because you're selecting something to be excluded which another platform may require.

Positive logic is the only way to deal with this.

We should instead have HAVE_IOPORT and platforms which need ISA/PCI IO space support should select this symbol instead - so it becomes an _inclusive_ feature rather than an _exclusive_ feature.

Russell,

Could you enlighten my very naive understanding of things about PCI/ISA IO space? On x86, I seem to understand this is the separate address space accessed by the special in/out CPU instructions. Are there ARM platforms with the same sort of things?

As far as I understand, on my ARM Marvell system, everything is memory-mapped, so there isn't such a separate PCI/ISA IO space.

Therefore, why would I need to "select HAVE_IOPORT" simply to be able to build libata-sff.c, that is used for PCI drivers that work fine with purely memory-mapped registers?

Sorry for the stupid/naive questions, but it'll definitely help to understand the matter.

Thanks,

Thomas

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

- PCI buses make the distinction between memory accesses and IO accesses.
- ARM doesn't have IO access instructions; everything is memory mapped.
- In order to make the two work together, whatever PCI interface provides a way to issue IO accesses via a contiguous memory mapped region.
- The PCI layer and drivers are written to keep these two spaces separate so that they remain portable across multiple different platforms.

Therefore, if you have PCI/ISA drivers that make use of IO space, you need to provide the IO space accessors and mapping functions so that these drivers can access the IO space on their devices in a platform independent manner.

An x86 processor has two address spaces

A memory space
An I/O space

They are both visible to the PCI bus and both transaction types are present on the bus and distinct.

There is if you ask the PCI bridge or beyond.

Because the drivers talk to the hardware beyond the bridge which knows the difference.

So on x86 we have

	inb(0xf00);

	generates an I/O transaction on the PCI bus for I/O 0xf00

	*0xf00

	generates a memory transacion on the PCI bus for memory 0xf00
	and the two are *not* the same thing or address

On many other platforms we have

	inb(0xf00)

	actually produces the equivalent of (*io_window+0xf00)

	which the PCI bridge turns into an I/O transaction for 0xf00

	(and for the out case may have an explicit barrier needed to get
	the I/O ordering rules)

	while

	*0xf00

	gets turned by the bridge into a memory transaction for 0xf00

The "no I/O space" case really applies to things like the S/390 mainframe which simply have no such concept on the system or the devices. In the ARM case the bus has an I/O space and the bridge glues the processors simpler model to the bridge model.

For compatibility reasons (so you an boot old software) the PCI bus also has some automatic magic mappings in I/O space, and a lot of LPC bus devices also have magic I/O mappings. For example a SATA controller in compatibility mode appears at a specified I/O address on the bus and is always on a specific IRQ (matching the old ISA PC mappings which map the old ISA MFM controller mappings whose offsets map compatiblity to the register mappings for the WD MFM controller used on the PC/AT)

In PC space this stuff is important and it's why up until Windows 7 era PC hardware you can boot DOS, as well as being why you can have one generic kernel for almost any PC hardware instead of the one per board problem some other platforms have.

Alan

While we are getting slightly offtopic, s390 is actually gaining PCI support
now: https://patchwork.kernel.org/patch/1740231/ For all I can tell, they would theoretically support an I/O space, but none of the supported add-on cards use it, so the kernel implementation doesn't need to bother.

One architecture that never has an I/O space is the arch/um, but that is special in a lot of ways, e.g. since it also never has MMIO.

On ARM, we have platforms that fall into four categories:

1. Full PCI or PCMCIA or ISA support with directly mapped I/O space

2. No PCI or PCMCIA or ISA support, and consequently no I/O space

3. PCI support but no I/O space because of limitations or bugs in the PCI hardware implementation.

4. ISA-style I/O space that is not offsettable (CONFIG_NO_IOPORT) but still has inb/outb accessors.

For cases 2 and 3, we can undefine the __io() macro, which leads to intentional build errors someone tries to build code that uses the inb/outb accessors. One missing piece that I have been working on in the past and been meaning to pick up again is a patch set to globally rename CONFIG_NO_IOPORT to the more appropriate CONFIG_NO_IOPORT_MAP, and introduce a new CONFIG_NO_IOPORT that signifies whether inb/outb are supported or not, rather than whether you can access the I/O ports through ioport_map() and ioread/iowrite.

Arnd

Exactly, that is what I meant. Sorry if I wasn't clear enough.

Right. Note that HAS_IOPORT already exists and is defined as

config HAS_IOPORT

        boolean
        depends on HAS_IOMEM && !NO_IOPORT
        default y

If we change it to

config HAS_IOPORT
boolean
depends on HAS_IOMEM
default !NO_IOPORT

then we can actually select both NO_IOPORT and HAS_IOPORT with the result of getting HAS_IOPORT. It is a bit confusing though to have both enabled, so we might still want to use an approach where we only select NO_IOPORT if we are sure that we can't have it.

Arnd

Dear Arnd Bergmann,

Indeed, but I guess the "select NO_IOPORT" on vexpress is here for a reason, no?

That said, unless I don't understand what IOPORTs are, I don't think my platform has any of them, so why should I "select HAVE_IOPORT" ?

This one is already being worked on by Fabio Estevam, see [PATCH v2]
ARM: Kconfig: Do not force selection of ARCH_VEXPRESS by ARCH_MULTI_V7.

arch/arm/mm/iomap.c is unconditionally compiled in all ARM kernels. And in this file, ioport_map() and ioport_unmap() are implement as soon as __io is defined. And basically, in arch/arm/include/asm/io.h, __io is defined for all platforms, except maybe on some platforms having their own mach/io.h file, but those are quite limited in number (ebsa110, rpc, at91, s3c24xx, pxa, omap1, footbridge and ixp4xx). So if __io is defined, says on VEXPRESS, why does it "select NO_IOPORT" ? Essentially all ARM platforms should select HAVE_IOPORT, except the few ones that don't define __io. Correct?

Thanks,

Thomas

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

That's an unfortunate side-effect of the single-zImage effort.

No they damned well should not; it actively _breaks_ at least some of the platforms you list above.

This whole area is a massive can of worms caused by people over the years just not understanding what an x86 IO port is and how to deal with it. It's very simple. The IO port space is for ISA/PCMCIA and PCI IO port regions. It is nothing more than that.

It's there because when you have a PCI peripheral in your system, you need to set it up, and the address space used for IO ports is entirely separate from that used for the rest of the memory mapped IO. You can't mix the IO ports amongst the MMIO ports - because most host PCI bridges will only let you map into MMIO space one region of PCI IO space.

The ISA case is a little harder to see, but believe me when I say that in the early days of Linux, we used talk to all ISA chips on platforms without ISA buses using the inb/outb/etc macros and it turned into one hell of a disgusting mess. Now these platforms all use the regular readb/writeb acros and ioremap() for their accesses which has resulted in much cleaner solutions - the result of that is these platforms no longer need the "ISA IO space" region, so providing ISA IO space support is additional unnecessary work for them.

Plus, if you _have_ IO space support, you must have some MMIO region for them to target - doing what many platforms have done to date and targetted ISA IO address 0 at virtual address 0 is just not on because as soon as you build a device driver which probes ISA addresses into your kernel, you will oops. Moreover, userspace can open /dev/ioport and ask the kernel to access IO port addresses that way too. Having IO space support enabled when the platform does not support it is a liability and an additional unnecessary security problem.

And on a lot of devices the LPC bus.

There shouldn't really be anything poking around that is modern - this is true of some PC stuff too.

In general however if its because you have a window partly mapped you could just catch the exception and load 0xFF for reads (and probably whine with a backtrace so you know who to moan at).

Alan

The problem that Russell refers to is that some platforms define a window that is 4GB large and starts at NULL. They then ioremap their PCI or PCMCIA I/O space window and use the virtual __iomem address as the offset into the 4GB I/O space window. Any driver (e.g. vgacon, /dev/port or some legacy alsa sound driver) that tries to access a low port number then ends up in user space memory, typically in an unmapped area there.

We definitely don't want to catch and fixup those exceptions. The solution is to change the platforms so they actually use the proper I/O window at a virtual address that is not used for anything else.

Arnd

Actually, I think that's a major bug in the single-zImage conversion:

#ifdef CONFIG_NEED_MACH_IO_H
#include <mach/io.h>
#elif defined(CONFIG_PCI)
#define IO_SPACE_LIMIT ((resource_size_t)0xfffff)
#define __io(a) __typesafe_io(PCI_IO_VIRT_BASE + ((a) & IO_SPACE_LIMIT))
#else
#define __io(a) __typesafe_io((a) & IO_SPACE_LIMIT)
#endif

#ifndef IO_SPACE_LIMIT
#if defined(CONFIG_PCMCIA_SOC_COMMON) || defined(CONFIG_PCMCIA_SOC_COMMON_MODULE) #define IO_SPACE_LIMIT ((resource_size_t)0xffffffff)
#elif defined(CONFIG_PCI) || defined(CONFIG_ISA) || defined(CONFIG_PCCARD) #define IO_SPACE_LIMIT ((resource_size_t)0xffff)
#else
#define IO_SPACE_LIMIT ((resource_size_t)0)
#endif
#endif

So, in the !PCI !PCMCIA !ISA !PCCARD case, IO_SPACE_LIMIT will be zero, which means the above common __io() functions will end up pointing the access at the NULL pointer - whereas we shouldn't be providing IO space support at all. (Okay, there's a couple of corner cases we've known about for a few _years_ but it's about time that stuff got fixed before we try and put yet more band-aids over this. Fix the root problems guys, don't create more problems.)

Correct. If HAS_IOPORT is not selected then we are potentially missing the dependent functions (because the platform has no IOPORT support) _or_ it does have ISA/PCI IO spaces _but_ they're not mappable via the ioport_map() mechanism due to some non-linearity involved in the translation.

To make that second point clear, that's platforms where:

	ioport_map(addr + 4) != ioport_map(addr) + 4.

For inb/inw and friends this shouldn't matter.

You can implement inl for example

as

	if (unlikely(addr & 3))
		// or in fact BUG on this for almost all h/w
		uninlined_hard_slow_inl(addr);
	else
		return *(u32 *)iospace_map[addr];

Exactly, and you wouldn't be using inb/inw with ioport_map().

The problem comes when you end up trying to deal with stuff which uses ioread{8,16} on ioport_map() cookies where it's assumed that adding N to the cookie is the same as adding N to the port address.

Thankfully those platforms aren't going to be a part of the single zImage project...

It's a cookie - this isn't a problem, you can support it via the mapping approah. Whether you want to is another question.

Don't blame you 8)

We're drifting off the topic of whether these things should be provided and whether the config symbols should be selected...

For a kernel configuration which includes platforms where there is are no ISA peripherals, no PCI bridge present, and no PCMCIA support, there's little point to having IO space support present.

However, the platform should still work correctly if IO space support is configured into the kernel (in other words, it shouldn't cause a failure) so that such a platform can co-operate with those platforms which do require IO space support.

Now, there was an idea a while back about having a common virtual address space to map PCI/ISA IO space to - which we have in the kernel - 1MB at 0xfee00000.

If PCI is disabled, the ends up being a 1:1 conversion between IO offset and CPU virtual address - that's partly there because no one's fixed up the soc-common PCMCIA stuff to dea with the PCMCIA socket IO spaces there. We really need to be fixing some of this stuff...

(Unfortunately, my SA11x0 platform with the dual PCMCIA/CF sockets is now my firewall so its out of the question to mess around with it on an ad-hoc basis.)

The Armada 370 has two gatable clocks for each PCIe interface, and we want both of them to be enabled. We therefore make one of the two clocks a child of the other, as we did for the sataX and sataXlnk clocks on Armada XP.

---
 drivers/clk/mvebu/clk-gating-ctrl.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index c6d3c26..a328be5 100644
@@ -119,8 +119,8 @@ static const struct mvebu_soc_descr __initconst armada_370_gating_descr[] = {
 	{ "pex1_en", NULL,  2 },
 	{ "ge1", NULL, 3 },
 	{ "ge0", NULL, 4 },
-	{ "pex0", NULL, 5 },
-	{ "pex1", NULL, 9 },
+	{ "pex0", "pex0_en", 5 },
+	{ "pex1", "pex1_en", 9 },
 	{ "sata0", NULL, 15 },
 	{ "sdio", NULL, 17 },
 	{ "tdm", NULL, 25 },

1.7.9.5

Instead of hardcoding "1" as being the bit value to enable an address decoding window, introduce and use a WIN_CTRL_ENABLE definition.

---
 arch/arm/plat-orion/addr-map.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index febe386..4dec3db 100644
@@ -38,6 +38,7 @@ EXPORT_SYMBOL_GPL(mv_mbus_dram_info);
  * CPU Address Decode Windows registers
  */
 #define WIN_CTRL_OFF		0x0000
+#define   WIN_CTRL_ENABLE       BIT(0)
 #define WIN_BASE_OFF		0x0004
 #define WIN_REMAP_LO_OFF	0x0008
 #define WIN_REMAP_HI_OFF	0x000c

@@ -79,7 +80,8 @@ void __init orion_setup_cpu_win(const struct orion_addr_map_cfg *cfg,
 	}
 
 	base_high = base & 0xffff0000;
-	ctrl = ((size - 1) & 0xffff0000) | (attr << 8) | (target << 4) | 1;
+	ctrl = ((size - 1) & 0xffff0000) | (attr << 8) | (target << 4) |
+		WIN_CTRL_ENABLE;
 
 	writel(base_high, addr + WIN_BASE_OFF);
 	writel(ctrl, addr + WIN_CTRL_OFF);

1.7.9.5

In the address decoding code, the orion_disable_wins() function is used at boot time to disable all address decoding windows, before configuring only the ones that are needed. This allows to make sure that no configuration is left from the bootloader.

As a preparation for the introduction of address decoding window allocation/deallocation function, we refactor this function into an orion_disable_cpu_win() which disables a single window.

The orion_config_wins() function is changed to call orion_disable_cpu_win() in a loop, to preserve an identical behavior.

---
 arch/arm/plat-orion/addr-map.c |   35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

index 4dec3db..dd98638 100644
@@ -95,6 +95,19 @@ void __init orion_setup_cpu_win(const struct orion_addr_map_cfg *cfg,
 	}
 }
 
+static void __init orion_disable_cpu_win(const struct orion_addr_map_cfg *cfg,
+					 const int win)
+{
+	void __iomem *addr = cfg->win_cfg_base(cfg, win);
+
+	writel(0, addr + WIN_BASE_OFF);
+	writel(0, addr + WIN_CTRL_OFF);
+	if (cfg->cpu_win_can_remap(cfg, win)) {
+		writel(0, addr + WIN_REMAP_LO_OFF);
+		writel(0, addr + WIN_REMAP_HI_OFF);
+	}
+}
+
 /*
  * Configure a number of windows.
  */

@@ -108,36 +121,22 @@ static void __init orion_setup_cpu_wins(const struct orion_addr_map_cfg * cfg,
 	}
 }
 
-static void __init orion_disable_wins(const struct orion_addr_map_cfg * cfg)
-{
-	void __iomem *addr;
-	int i;
-
-	for (i = 0; i < cfg->num_wins; i++) {
-		addr = cfg->win_cfg_base(cfg, i);
-
-		writel(0, addr + WIN_BASE_OFF);
-		writel(0, addr + WIN_CTRL_OFF);
-		if (cfg->cpu_win_can_remap(cfg, i)) {
-			writel(0, addr + WIN_REMAP_LO_OFF);
-			writel(0, addr + WIN_REMAP_HI_OFF);
-		}
-	}
-}
-
 /*
  * Disable, clear and configure windows.
  */
 void __init orion_config_wins(struct orion_addr_map_cfg * cfg,
 			      const struct orion_addr_map_info *info)
 {
+	int win;
+
 	if (!cfg->cpu_win_can_remap)
 		cfg->cpu_win_can_remap = orion_cpu_win_can_remap;
 
 	if (!cfg->win_cfg_base)
 		cfg->win_cfg_base = orion_win_cfg_base;
 
-	orion_disable_wins(cfg);
+	for (win = 0; win < cfg->num_wins; win++)
+		orion_disable_cpu_win(cfg, win);
 
 	if (info)
 		orion_setup_cpu_wins(cfg, info);

1.7.9.5

In the address decoding code, we implement two new functions:
orion_alloc_cpu_win() and orion_free_cpu_win(). The first function finds an unused address decoding window, and configures it according to the given arguments (in terms of base address, size, target, attributes). The second function frees an address decoding window, given a physical base address.

Those two new functions will be used by the PCIe code, which needs to dynamically register address decoding windows depending on the PCIe devices that are detected.

The orion_free_cpu_win() function is only here to handle error cases in the PCIe devices initialization, in the normal case, address decoding windows are never freed.

---
 arch/arm/plat-orion/addr-map.c              |   50 +++++++++++++++++++++++++++
 arch/arm/plat-orion/include/plat/addr-map.h |    7 ++++
 2 files changed, 57 insertions(+)

index dd98638..8f4ef82 100644
@@ -109,6 +109,56 @@ static void __init orion_disable_cpu_win(const struct orion_addr_map_cfg *cfg,
 }
 
 /*
+ * Find an unused address decoding window, and enable it according to
+ * the arguments passed (base, size, target, attributes, remap).
+ */
+int __init orion_alloc_cpu_win(const struct orion_addr_map_cfg *cfg,
+			       const u32 base, const u32 size,
+			       const u8 target, const u8 attr, const int remap)
+{
+	int win;
+
+	for (win = 0; win < cfg->num_wins; win++) {
+		void __iomem *addr = cfg->win_cfg_base(cfg, win);
+		u32 ctrl = readl(addr + WIN_CTRL_OFF);
+		if (!(ctrl & WIN_CTRL_ENABLE))
+			break;
+	}
+
+	/* No more windows available */
+	if (win == cfg->num_wins)
+		return -ENOMEM;
+
+	orion_setup_cpu_win(cfg, win, base, size, target, attr, remap);
+	return 0;
+}
+
+/*
+ * Free an address decoding window, given its base address.
+ */
+int __init orion_free_cpu_win(const struct orion_addr_map_cfg *cfg,
+			      const u32 base)
+{
+	int win;
+
+	for (win = 0; win < cfg->num_wins; win++) {
+		void __iomem *addr = cfg->win_cfg_base(cfg, win);
+		u32 winbase = readl(addr + WIN_BASE_OFF);
+		u32 ctrl = readl(addr + WIN_CTRL_OFF);
+
+		if (!(ctrl & WIN_CTRL_ENABLE))
+			continue;
+
+		if (winbase == (base & 0xffff0000)) {
+			orion_disable_cpu_win(cfg, win);
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+/*
  * Configure a number of windows.
  */
 static void __init orion_setup_cpu_wins(const struct orion_addr_map_cfg * cfg,

index b76c065..f8bb539 100644
@@ -49,6 +49,13 @@ void __init orion_setup_cpu_win(const struct orion_addr_map_cfg *cfg,
 				const u32 size, const u8 target,
 				const u8 attr, const int remap);
 
+int __init orion_alloc_cpu_win(const struct orion_addr_map_cfg *cfg,
+			       const u32 base, const u32 size,
+			       const u8 target, const u8 attr, const int remap);
+
+int __init orion_free_cpu_win(const struct orion_addr_map_cfg *cfg,
+			      const u32 base);
+
 void __init orion_setup_cpu_mbus_target(const struct orion_addr_map_cfg *cfg,
 					const void __iomem *ddr_window_cpu_base);
 #endif

1.7.9.5

Hi Thomas

Dear Andrew Lunn,

Ok, good idea, will do.

Thanks,

Thomas

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

This commit adds two functions armada_370_xp_alloc_pcie_window() and armada_370_xp_free_pcie_window() that respectively allocate and free an address decoding window pointing to either a memory or I/O region of a PCIe device.

Those functions will be used by the PCIe driver to create and remove those regions depending on the PCIe devices that are detected.

---
 arch/arm/mach-mvebu/addr-map.c |  156 ++++++++++++++++++++++++++++++++++++++--
 arch/arm/mach-mvebu/common.h   |    4 ++
 2 files changed, 156 insertions(+), 4 deletions(-)

index ab9b3bd..c21aa7b 100644
@@ -24,14 +24,10 @@
 #define ARMADA_XP_TARGET_DEV_BUS	1
 #define   ARMADA_XP_ATTR_DEV_BOOTROM    0x1D
 #define ARMADA_XP_TARGET_ETH1		3
-#define ARMADA_XP_TARGET_PCIE_0_2	4
 #define ARMADA_XP_TARGET_ETH0		7
-#define ARMADA_XP_TARGET_PCIE_1_3	8
 
 #define ARMADA_370_TARGET_DEV_BUS       1
 #define   ARMADA_370_ATTR_DEV_BOOTROM   0x1D
-#define ARMADA_370_TARGET_PCIE_0        4
-#define ARMADA_370_TARGET_PCIE_1        8
 
 #define ARMADA_WINDOW_8_PLUS_OFFSET       0x90
 #define ARMADA_SDRAM_ADDR_DECODING_OFFSET 0x180

@@ -89,6 +85,158 @@ static struct __initdata orion_addr_map_cfg addr_map_cfg = {
 	.win_cfg_base = armada_cfg_base,
 };
 
+#ifdef CONFIG_PCI
+/*
+ * PCIe windows allocation code.
+ */
+#define ARMADA_370_XP_PCIE_MEM_START 0xC0000000
+#define ARMADA_370_XP_PCIE_MEM_END   (ARMADA_370_XP_PCIE_MEM_START + SZ_256M)
+#define ARMADA_370_XP_PCIE_IO_START  0xF2000000
+#define ARMADA_370_XP_PCIE_IO_END    (ARMADA_370_XP_PCIE_IO_START + SZ_1M)
+
+static unsigned long armada_370_xp_pcie_memaddr = ARMADA_370_XP_PCIE_MEM_START;
+static unsigned long armada_370_xp_pcie_ioaddr = ARMADA_370_XP_PCIE_IO_START;
+
+/*
+ * This structure and the following arrays allow to map a PCIe (port,
+ * lane) tuple to the corresponding (target, attribute) tuple needed
+ * to configure an address decoding window for the given PCIe (port,
+ * lane).
+ */
+struct pcie_mapping {
+	int port;
+	int lane;
+	u8  target;
+	u8  attr;
+};
+
+struct pcie_mapping armada_xp_pcie_mappings[] = {
+	{ .port = 0, .lane = 0, .target = 4, .attr = 0xE0 },
+	{ .port = 0, .lane = 1, .target = 4, .attr = 0xD0 },
+	{ .port = 0, .lane = 2, .target = 4, .attr = 0xB0 },
+	{ .port = 0, .lane = 3, .target = 4, .attr = 0x70 },
+	{ .port = 1, .lane = 0, .target = 8, .attr = 0xE0 },
+	{ .port = 1, .lane = 1, .target = 8, .attr = 0xD0 },
+	{ .port = 1, .lane = 2, .target = 8, .attr = 0xB0 },
+	{ .port = 1, .lane = 3, .target = 8, .attr = 0x70 },
+	{ .port = 2, .lane = 0, .target = 4, .attr = 0xF0 },
+	{ .port = 3, .lane = 0, .target = 8, .attr = 0xF0 },
+	{ .port = -1 },
+};
+
+struct pcie_mapping armada_370_pcie_mappings[] = {
+	{ .port = 0, .lane = 0, .target = 4, .attr = 0xE0 },
+	{ .port = 1, .lane = 0, .target = 8, .attr = 0xE0 },
+	{ .port = -1 },
+};
+
+/*
+ * This function finds an available physical address range between
+ * ARMADA_370_XP_PCIE_MEM_START and ARMADA_370_XP_PCIE_MEM_END (for
+ * PCIe memory regions) or between ARMADA_370_XP_PCIE_IO_START and
+ * ARMADA_370_XP_PCIE_IO_END (for PCIe I/O regions) and creates an
+ * address decoding window from this allocated address pointing to the
+ * right PCIe device.
+ *
+ * An error code is returned, the allocated base address is returned
+ * through the 'outbase' argument.
+ */
+int __init armada_370_xp_alloc_pcie_window(int pcie_port, int pcie_lane,
+					   int type, u32 size,
+					   unsigned long *outbase)
+{
+	struct pcie_mapping *mapping, *mappings;
+	u8 target, attr;
+	u32 base;
+	int ret;
+
+	if (of_machine_is_compatible("marvell,armadaxp"))
+		mappings = armada_xp_pcie_mappings;
+	else if (of_machine_is_compatible("marvell,armada370"))
+		mappings = armada_370_pcie_mappings;
+	else
+		return -ENODEV;
+
+	for (mapping = mappings; mapping->port != -1; mapping++)
+		if (mapping->port == pcie_port && mapping->lane == pcie_lane)
+			break;
+
+	if (mapping->port == -1)
+		return -ENODEV;
+
+	target = mapping->target;
+	attr = mapping->attr;
+
+	if (type == IORESOURCE_MEM) {
+		/*
+		 * Bit 3 of the attributes indicates that it is a
+		 * memory region, as opposed to an I/O region
+		 */
+		attr |= (1 << 3);
+
+		if (armada_370_xp_pcie_memaddr + size >
+		    ARMADA_370_XP_PCIE_MEM_END)
+			return -ENOMEM;
+
+		base = armada_370_xp_pcie_memaddr;
+		armada_370_xp_pcie_memaddr += size;
+
+		ret = orion_alloc_cpu_win(&addr_map_cfg, base, size, target,
+					  attr, -1);
+		if (ret) {
+			armada_370_xp_pcie_memaddr -= size;
+			return ret;
+		}
+	} else if (type == IORESOURCE_IO) {
+		if (armada_370_xp_pcie_ioaddr + size >
+		    ARMADA_370_XP_PCIE_IO_END)
+			return -ENOMEM;
+
+		base = armada_370_xp_pcie_ioaddr;
+		armada_370_xp_pcie_ioaddr += size;
+
+		ret = orion_alloc_cpu_win(&addr_map_cfg, base, size, target,
+					  attr, -1);
+		if (ret) {
+			armada_370_xp_pcie_ioaddr -= size;
+			return ret;
+		}
+	} else
+		return -ENODEV;
+
+	*outbase = base;
+	return 0;
+}
+
+/*
+ * Frees an address decoding window previously allocated by
+ * armada_370_xp_alloc_pcie_window(). Note that only the last window
+ * allocated for a given type (MEM or IO) can be freed, due to the
+ * simplicity of the allocator. This is however sufficient to handle
+ * the error cases when initializing one PCIe device.
+ */
+int __init armada_370_xp_free_pcie_window(int type, unsigned long base,
+					  u32 size)
+{
+	if (type == IORESOURCE_MEM) {
+		/* We can only free the last allocated window */
+		if (base + size != armada_370_xp_pcie_memaddr)
+			return -EINVAL;
+		orion_free_cpu_win(&addr_map_cfg, base);
+		armada_370_xp_pcie_memaddr -= size;
+	} else if (type == IORESOURCE_IO) {
+		/* We can only free the last allocated window */
+		if (base + size != armada_370_xp_pcie_ioaddr)
+			return -EINVAL;
+		orion_free_cpu_win(&addr_map_cfg, base);
+		armada_370_xp_pcie_ioaddr -= size;
+	} else
+		return -EINVAL;
+
+	return 0;
+}
+#endif
+
 static int __init armada_setup_cpu_mbus(void)
 {
 	struct device_node *np;

index aa27bc2..3cef81a 100644
@@ -25,4 +25,8 @@ int armada_370_xp_coherency_init(void);
 int armada_370_xp_pmsu_init(void);
 void armada_xp_secondary_startup(void);
 extern struct smp_operations armada_xp_smp_ops;
+
+int armada_370_xp_alloc_pcie_window(int pcie_port, int pcie_lane,
+				    int type, u32 size, unsigned long *outbase);
+int armada_370_xp_free_pcie_window(int type, unsigned long base, u32 size);
 #endif

1.7.9.5

mvebu is a new-style Orion platform, so it only selects PLAT_ORION, but not PLAT_ORION_LEGACY. It will however need the common PCIe code from plat-orion, so make this code available for PLAT_ORION platforms as a whole, and not only PLAT_ORION_LEGACY platforms.

We also take this opportunity to build the PCIe code only when CONFIG_PCI is enabled.

---
 arch/arm/plat-orion/Makefile |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index a82cecb..1aca22b 100644
@@ -4,7 +4,8 @@
 ccflags-$(CONFIG_ARCH_MULTIPLATFORM) := -I$(srctree)/$(src)/include
 
 obj-y                             += addr-map.o
+obj-$(CONFIG_PCI)                 += pcie.o
 
 orion-gpio-$(CONFIG_GENERIC_GPIO) += gpio.o
-obj-$(CONFIG_PLAT_ORION_LEGACY)   += irq.o pcie.o time.o common.o mpp.o
+obj-$(CONFIG_PLAT_ORION_LEGACY)   += irq.o time.o common.o mpp.o
 obj-$(CONFIG_PLAT_ORION_LEGACY)   += $(orion-gpio-y)

1.7.9.5

This driver implements the hw_pci operations needed by the core ARM PCI code to setup PCI devices and get their corresponding IRQs, and the pci_ops operations that are used by the PCI core to read/write the configuration space of PCI devices.

In addition, this driver enumerates the different PCIe slots, and for those having a device plugged in, it allocates the necessary address decoding windows, using the new armada_370_xp_alloc_pcie_window() function from mach-mvebu/addr-map.c.

---
 .../devicetree/bindings/pci/armada-370-xp-pcie.txt |  136 +++++++++
 arch/arm/mach-mvebu/Makefile                       |    1 +
 arch/arm/mach-mvebu/pcie.c                         |  306 ++++++++++++++++++++
 3 files changed, 443 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/armada-370-xp-pcie.txt
 create mode 100644 arch/arm/mach-mvebu/pcie.c

new file mode 100644
index 0000000..daf0775
@@ -0,0 +1,136 @@
+* Marvell Armada 370/XP PCIe interfaces
+
+Mandatory properties:
+- compatible: must be "marvell,armada-370-xp-pcie"
+- status: either "disabled" or "okay"
+- #address-cells, set to <1>
+- #size-cells, set to <1>
+- ranges: describes the association between the physical addresses of
+  the PCIe registers for each PCIe interface with "virtual" addresses
+  as seen by the sub-nodes. One entry per PCIe interface. Each entry
+  must have 3 values: the "virtual" address seen by the sub-nodes, the
+  real physical address of the PCIe registers, and the size.
+
+In addition, the Device Tree node must have sub-nodes describing each
+PCIe interface, having the following mandatory properties:
+- reg: the address and size of the PCIe registers (translated
+  addresses according to the ranges property of the parent)
+- interrupts: the interrupt number of this PCIe interface
+- clocks: the clock associated to this PCIe interface
+- marvell,pcie-port: the physical PCIe port number
+- status: either "disabled" or "okay"
+
+and the following optional properties:
+- marvell,pcie-lane: the physical PCIe lane number, for ports having
+  multiple lanes. If this property is not found, we assume that the
+  value is 0.
+
+Example:
+
+pcie-controller {
+	compatible = "marvell,armada-370-xp-pcie";
+	status = "disabled";
+	#address-cells = <1>;
+	#size-cells = <1>;
+	ranges = <0       0xd0040000 0x2000 /* port0x1_port0 */
+		  0x2000  0xd0042000 0x2000 /* port2x1_port0 */
+		  0x4000  0xd0044000 0x2000 /* port0x1_port1 */
+		  0x8000  0xd0048000 0x2000 /* port0x1_port2 */
+		  0xC000  0xd004C000 0x2000 /* port0x1_port3 */
+		  0x10000 0xd0080000 0x2000 /* port1x1_port0 */
+		  0x12000 0xd0082000 0x2000 /* port3x1_port0 */
+		  0x14000 0xd0084000 0x2000 /* port1x1_port1 */
+		  0x18000 0xd0088000 0x2000 /* port1x1_port2 */
+		  0x1C000 0xd008C000 0x2000 /* port1x1_port3 */>;
+
+	pcie0.0 at 0xd0040000 {
+		reg = <0x0 0x2000>;
+		interrupts = <58>;
+		clocks = <&gateclk 5>;
+		marvell,pcie-port = <0>;
+		marvell,pcie-lane = <0>;
+		status = "disabled";
+	};
+
+	pcie0.1 at 0xd0044000 {
+		reg = <0x4000 0x2000>;
+		interrupts = <59>;
+		clocks = <&gateclk 5>;
+		marvell,pcie-port = <0>;
+		marvell,pcie-lane = <1>;
+		status = "disabled";
+	};
+
+	pcie0.2 at 0xd0048000 {
+		reg = <0x8000 0x2000>;
+		interrupts = <60>;
+		clocks = <&gateclk 5>;
+		marvell,pcie-port = <0>;
+		marvell,pcie-lane = <2>;
+		status = "disabled";
+	};
+
+	pcie0.3 at 0xd004C000 {
+		reg = <0xC000 0x2000>;
+		interrupts = <61>;
+		clocks = <&gateclk 5>;
+		marvell,pcie-port = <0>;
+		marvell,pcie-lane = <3>;
+		status = "disabled";
+	};
+
+	pcie1.0 at 0xd0040000 {
+		reg = <0x10000 0x2000>;
+		interrupts = <62>;
+		clocks = <&gateclk 6>;
+		marvell,pcie-port = <1>;
+		marvell,pcie-lane = <0>;
+		status = "disabled";
+	};
+
+	pcie1.1 at 0xd0044000 {
+		reg = <0x14000 0x2000>;
+		interrupts = <63>;
+		clocks = <&gateclk 6>;
+		marvell,pcie-port = <1>;
+		marvell,pcie-lane = <1>;
+		status = "disabled";
+	};
+
+	pcie1.2 at 0xd0048000 {
+		reg = <0x18000 0x2000>;
+		interrupts = <64>;
+		clocks = <&gateclk 6>;
+		marvell,pcie-port = <1>;
+		marvell,pcie-lane = <2>;
+		status = "disabled";
+	};
+
+	pcie1.3 at 0xd004C000 {
+		reg = <0x1C000 0x2000>;
+		interrupts = <65>;
+		clocks = <&gateclk 6>;
+		marvell,pcie-port = <1>;
+		marvell,pcie-lane = <3>;
+		status = "disabled";
+	};
+
+	pcie2 at 0xd0042000 {
+		reg = <0x2000 0x2000>;
+		interrupts = <99>;
+		clocks = <&gateclk 7>;
+		marvell,pcie-port = <2>;
+		marvell,pcie-lane = <0>;
+		status = "disabled";
+	};
+
+	pcie3 at 0xd0082000 {
+		reg = <0x12000 0x2000>;
+		interrupts = <103>;
+		clocks = <&gateclk 8>;
+		marvell,pcie-port = <3>;
+		marvell,pcie-lane = <0>;
+		status = "disabled";
+	};
+};
+

index 5dcb369..e458ede 100644
@@ -5,3 +5,4 @@ obj-y += system-controller.o
 obj-$(CONFIG_MACH_ARMADA_370_XP) += armada-370-xp.o irq-armada-370-xp.o addr-map.o coherency.o coherency_ll.o pmsu.o
 obj-$(CONFIG_SMP)                += platsmp.o headsmp.o
 obj-$(CONFIG_HOTPLUG_CPU)        += hotplug.o
+obj-$(CONFIG_PCI)		 += pcie.o

1.7.9.5

Dear Andrew Lunn,

Indeed.

Sure.

I indeed plan to do more refactoring. In the end, my goal is to have one single PCIe driver for kirkwood/mvebu/etc, but let's do it step by step :-)

Best regards,

Thomas

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

It seems that this is not following the IEEE PCI bindings, which will cause problems as soon as you try to connect PCI cards with bridges bto it.

Dear Arnd Bergmann,

[...]

As the "RFC" suggested in the title, I was of course pretty sure that the DT binding was not correct. However, the reason I did the DT binding the way I did it is because I really don't understand how to map the DT binding visible at http://devicetree.org/MPC5200:PCI with the hardware we have. I'll need a little bit of guidance to understand how to map our hardware features to such a DT binding. So let me describe briefly how the Marvell PCIe stuff works, by taking example on the Armada XP.

Armada XP supports 10 PCIe interfaces, each of them can physically correspond to a PCIe slot on the board in which you can plug a PCIe device.

Each of those PCIe interfaces is associated to a range of registers inside the SoC that are used to see if the PCIe link is up or down on this interface, access the PCIe configuration space of the device connected on this interface, get the status of PCIe interrupts, etc. Those registers are available at physical addresses 0xD004000, 0xD0042000, 0xD0044000, 0xD0048000, 0xD004C000, 0xD008000, 0xD0082000, 0xD0084000, 0xD0088000, 0xD008C000 (one address = one PCIe interface). I need to ioremap() those registers in order to interact with the PCIe interface, and that's the reason why I have put these addresses in the reg = <...> property of the DT sub-nodes in my proposal.

Then, to access the PCIe device memory and I/O BARs, there are no fixed addresses in the CPU physical address space. The Armada XP (and other earlier Marvell SoCs) have a number of configurable address decoding windows. Those windows allow the programmer to tell the CPU that a window of physical addresses starting at address X of size S points to device D (where device D can be "PCIe interface n?8"). So, in my driver, I set up one PCI memory window and one PCI I/O window per PCIe device dynamically depending on which PCIe interfaces actually have a device connected to them. Since those windows are limited in number, and the global physical address space is limited in size, we definitely do not want to have static mappings at fixed physical addresses hardcoded in the Device Tree.

That's my main problem: the DT binding at
http://devicetree.org/MPC5200:PCI seems to assume that the PCIe memory and I/O BARs are accessible at fixed CPU physical addresses. This is definitely not the case on Armada XP: the CPU physical address for a given device will change from one boot to another depending on how many PCIe interfaces actually have a device plugged into them. And we clearly do not want to make those addresses fixed and static. If you could guide me on how the approach to make those standard DT bindings compatible with the specificities of the Marvell hardware, it'd be nice.

Same thing for the "interrupt mapping" thing: the "unit interrupt specifier" is explained at http://devicetree.org/MPC5200:PCI as "The only missing part for now are the weird numbers int the PCI bus unit interrupt specifier. Now we have to look into the schematics[6] of the Lite5200b board. Here you can see that the IDSEL line of PCI slot 1 is tied to PCI address line 24 and the IDSEL line of PCI slot 2 to PCI address line 25[7]. The IDSEL lines are used to determine the PCI device number." Again, we don't have such hardcoded relation between a PCIe interface and "PCI address lines", so I really don't see how to use this "interrupt mapping" representation.

Unfortunately, for now, the binding at
http://devicetree.org/MPC5200:PCI does not make any sense to me compared to the hardware that we have. I would definitely appreciate if someone could enlighten me on how to properly use such bindings, considering the specificities of the hardware we have.

Of course, do not hesitate to get back to me if you need further details about the hardware.

Thanks,

Thomas

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Unfortunately DT pretty much requires that all PCI host bridges hard code the addresess. The expectation seems in many cases that the BIOS will do assignment and discovery and just propagate it into the DT format.

If you are able to make the driver appear as one host bridge with multiple root ports, (as we have been discussion, one way other the other) then the DT model is fairly sane. The ranges property specifies the entire available decode address space and the driver/linux core/whatever has to split that up amongst the ports and assign the split up to the mbus windows.

pex at e0000000 {
                /* The standard for PCI devices is 3 byte addresses, with the top
                   cell being the region encoding 0x02000000 is non prefetchable mmio */
                device_type = "pci";
                #address-cells = <3>;
                #size-cells = <2>;
                ranges = <0x02000000 0x00000000 0x00000000  0xe0000000  0x0 0x8000000>;
		/* Also ranges for prefetchable and IO. On Marvell
		   prefetchable and MMIO are the same decoder window */
                bus-range = <0x0 0xFF>;

		regs = <0xD0040000 0x2000
		        0xD0042000 0x2000
                        ...>; // One tuple for each port
                interrupts = <58 59.. >; // One IRQ for each port

		/* The PEX is a chained interrupt controller too,
		   since each pex has a interrupt mask/cause that
		   contains INTA/B/C/D and internal status */
		interrupt-controller;

		/* This describes the INTA/B/C/D interrupt lines for
		   each port. The exact layout depends on how the
		   ports are viewed in the kernel */
		interrupt-map = <.. &pex ..>;
};

[ you may need to pull the pex interrupt controller out, but that is,
IHMO, not as easy to work with considering how the HW is setup]

With your current driver that has one host bridge per PEX you need one of the above for every PEX with ranges that describe the static allocation for the PEX.

A combined multi-port driver would use something like the above, with a regs tuple for each port.

The driver should parse the ranges and bus-range and setup the resource regions appropriately, be it for the port or for the whole port group.

The document is describing the legacy bus PCI interrupt scheme. PCI-E emulates this using INTA/B/C/D inband messaging. It also emulates the 'standard' IDSEL hookup. So you end up with a very simple identity mapping..

/* Single port version, multiple-ports will have one group of four
for each */
interrupt-map-mask = <0x0 0x0 0x0 0x7>;
interrupt-map = <

                     0x0 0x0 0x0 0x1 &pex 0> /* int A */
                     0x0 0x0 0x0 0x2 &pex 1> /* int B */
                     0x0 0x0 0x0 0x3 &pex 2> /* int C */
                     0x0 0x0 0x0 0x4 &pex 3> /* int D */>;

However, be aware, that if you insert a bridge into your software model (as discussed) then you probably want to describe the bridge in the DTS and move the interrupt-map into the bridge itself so the bridge required twizzling is not as complex.

The process that goes on behind the scences is roughly:
- Start a device asking for an interrupt
- Construct the 4 dw UIS
- Travel up the bridges, swizzling the last dw according to the bridge rules
- When we finally find a node with interrupt-map, match the 4 dw UIS to get to a OF interrupt handle

Note that the Kirkwood/Orion stuff doesn't really do this right. It just assigns a single interrupt to the port, which is the PEX interrupt cause summary interrupt and provides no possibility to access INTA/B/C/D which will break if you ever try to use a multi-function device or a PCI bridge. It also doesn't monitor the other PEX interrupt statuses to machine check when things go wrong on the bus.

Also, I have some stuff that makes MSI work on Kirkwood that I am using, you might be interested in some of that too someday..

Well, I hope you find this informative..

Jason

Agreed, this looks like it would be the ideal representation.

If the IntA to IntD lines are all on the same host interrupt, you might only need one line above and make the map-mask all zeroes.

I would assume though that we need to key in the slot ID in some way.

Thank you very much for your explanation, that was much more than I would have known without extensive research on the Marvell specifics.

Arnd

Well, no that is where the existing stuff goes wrong..

Each of the four INTx's on each port need a dedicated linux interrupt vector. Sharing interrupts is bad. The chip has dedicated cause and mask bits so there is no reason to share.

To do this the pex driver has to allocate 4 irq descs per port, setup a generic irq chip for the driver, use irq_set_chained_handler on the summary interrupt in the main cause register and then decode the INTx bits in the chained handler function. This is very straightforward and very much worth doing.

Since this makes the entire interrupt mapping completely internal to the driver there is no real need for the DT properties. The driver just needs to hook the PCIe core to ensure it gets called when irq translation happens across the root port bridge. This is very similar to how an IOAPIC works in x86 land.

With PCIe there is no possible board specific variation of interrupts. INTx messages are always in-band, and they are always captured by the PEX's interrupt cause register. With no variation there is no need to configure it in DT.

Jason

Dear Jason Gunthorpe,

Yes, this is definitely part of my plans. For now, I kept the existing PCIe implementation of Kirkwood and al with regard to IRQs, but when I read more about INT{A,B,C,D} and the cause register that is available, I immediately noted in my TODO-list: use a generic irq chip to demultiplex those IRQ events.

Best regards,

Thomas

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

The DT is not fixed. You can make run-time changes if needed. As I mentioned in the other thread on PCI and DT, you could set or update the ranges property within your PCI host driver with the addresses you end up using. Or omitting the ranges property would also work if you have no child devices (which is typical).

Rob

Do those ten ports have a shared I/O space window, or does each one have its own 64K?

Similarly, how is the config space set up: Does each port have its own config space with 256 buses, or is there a fixed (or variable) mapping of bus numbers to ports?

But would it be possible to put the mapping in the device tree? When you describe a particular board, you probably know which devices are connected to it, so it could all be put in the device tree and set up at boot time based on the values in there.

Setting up the windows at run-time sounds very complicated unless you know exactly which physical addresses are available, so I think you should at least have information in the device tree which addresses could be used, in some form, but leave out the mapping for the buses that you cannot yet assign based on the information in the dts file.

As Jason alrready commented, the hardwiring of the lines does not apply for PCIe, which uses interrupt messages, but the IntA mapping should still be documented in the spec for the PCIe host.

Arnd

The smallest mbus routing window size is 64k, and you need to use a mbus routing window per PEX to decode into IO.

Thomas: Note this complicates my earlier suggestion of just using config transactions because this is horribly non-conformant, PCIe requires a much finer granularity for the root port bridge, it would need some kind of specialness to make the standard Linux resource allocator work properly.... :|

Each PEX has it's own configuration cycle generation register, and is completely independent of other PEX's. They could be seperate domains in Linux, but lets not do that :)

Marvell did not design a root complex for this chip. The PEX is an end port core with some additional stuff bodged on to handle root port TLP processing requirements, but none of the other root port and root complex requirements are met.

Ideally SW could bodge it into a sort of compliant root complex, but that is looking like some level of core PCI changes would be needed..

By far the easiest thing is to keep them as separate PCI busses and require DT to manage each one individually, address ranges and all. :(

Jason

Dear Jason Gunthorpe,

Correct.

Hum, not sure to follow you here. What sort of finer granularity does PCIe requires?

Does that mean that your earlier suggestion of emulating a PCI-to-PCI bridge in software is no longer your preferred suggestion?

Thomas

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

It's common for multiple buses to share a single I/O address space, and just allocate port numbers for each BAR from a global 64KB window, because x86 only has a single 64KB I/O space in hardware.

Maybe it works correctly if you set up all ten I/O windows to point to the same addresses? I don't have the documentation, so it might say that this is unsupported, but otherwise it may be worth trying.

If the child buses of that virtual bridge can't use the same I/O space window, that would require significant changes to the Linux PCI implementation, which does not sound right.

One possibility would be to declare I/O space accesses invalid on this driver, but that would break support for a number of (older) devices.

Arnd

PCIe requires 4k alignment of bridge IO addresses and 1M alignment of bridge memory addresses.. I was thinking the mismatch of 4k vs 64k in the mbus kills the idea, but some cleverness with the VM is possible to fix it up. See below

The kirkwood docs say windows must never overlap.

The default value for ARM's IO_SPACE_LIMIT is 1048575 so can fit 16 64k IO regions within PCI_IO_VIRT_BASE space. So far the marvell drivers have assigned a unique 64k io region to each PEX.

The Linux PCI implementation has no problem with a > 16 bit IO address, it just truncates it when it goes into configuration registers, which matches what the HW does.

However, if you want to make each PEX into a compliant virtual root port bridge then you have to live with PCIe rules, which means the host bridge gets a 64k region and each virtual root port bridge gets configured for a 4k aligned sub region.

So you have to match this to the 64k IO decoding windows that marvell supports.

Here is a possible way using the VM subsystem:

- You reserve 10*64k of physical address space for bridge IO decoding
- The Host bridge and linux are told to map only 64k of IO from 0 to 0xFFFF
- When linux asks the root port bridge to allocate an IO range it is aligned to a 4K boundary (PCIe requires this)
- The root port bridge grabs an mbus window and one of the 64k physical blocks and sets that up.
- The root port bridge uses pci_ioremap_io to assign the virtual addresses for the 4k aligned range that was assigned by linux to a portion of the physical addresses within the 64k physical window.

eg:
PEX 0: Physical IO window 0x10000 -> 0x1ffff
PEX 1: Physical IO window 0x20000 -> 0x2ffff

PEX 0: Bridge is configured to claim IO range 0x0 -> 0xfff PEX 1: Bridge is configured to claim IO range 0x3000 -> 0x3fff

pci_ioremap_io does:
PCI_IO_VIRT_BASE + 0 -> 0xfff == physical 0x10000 -> 0x10fff
PCI_IO_VIRT_BASE + 0x3000 -> 0x3fff == physical 0x23000 -> 0x23fff

So via pci_ioremap_io we stitch together a single 64k IO space across all 10 PEX's using page granular portions of the physical 640K allocated for the mbus mapping.

It would be a bit tidier if the PCIe core code could learn that these particular root port bridges have a 64k alignment requirement for IO, but I didn't see any easy way to do that .... ?

Regards,
Jason

Now that the PCIe driver for mvebu has been integrated and all its relevant dependencies, we can mark the ARCH_MVEBU platform has MIGHT_HAVE_PCI, which allows to select the PCI bus support if needed.

---
 arch/arm/mach-mvebu/Kconfig |    2 ++
 1 file changed, 2 insertions(+)

index 440b13e..f12e475 100644
@@ -13,6 +13,8 @@ config ARCH_MVEBU
 	select MVEBU_CLK_CORE
 	select MVEBU_CLK_CPU
 	select MVEBU_CLK_GATING
+	select MIGHT_HAVE_PCI
+	select PCI_QUIRKS if PCI
 
 if ARCH_MVEBU
 

1.7.9.5

The Armada 370 SoC has two 1x PCIe 2.0 interfaces, so we add the necessary Device Tree informations to make these interfaces availabel.

---
 arch/arm/boot/dts/armada-370.dtsi |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

index 636cf7d..2e3c8e0 100644
@@ -132,5 +132,30 @@
 				dmacap,memset;
 			};
 		};
+
+		pcie-controller {
+			compatible = "marvell,armada-370-xp-pcie";
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0      0xd0040000 0x2000
+			          0x2000 0xd0080000 0x2000>;
+
+			pcie0 at 0xd0040000 {
+				reg = <0x0 0x2000>;
+				interrupts = <58>;
+				clocks = <&gateclk 5>;
+				status = "disabled";
+				marvell,pcie-port = <0>;
+			};
+
+			pcie1 at 0xd0080000 {
+				reg = <0x2000 0x2000>;
+				interrupts = <62>;
+				clocks = <&gateclk 9>;
+				status = "disabled";
+				marvell,pcie-port = <1>;
+			};
+		};
 	};
 };

1.7.9.5

The Armada XP SoCs have multiple PCIe interfaces. The MV78230 has 2 PCIe units (one 4x or quad 1x, the other 1x only), the MV78260 has 3 PCIe units (two 4x or quad 1x and one 4x/1x), the MV78460 has 4 PCIe units (two 4x or quad 1x and two 4x/1x). We therefore add the necessary Device Tree informations to make those PCIe interfaces usable.

---
 arch/arm/boot/dts/armada-xp-mv78230.dtsi |   62 +++++++++++++++++
 arch/arm/boot/dts/armada-xp-mv78260.dtsi |   72 +++++++++++++++++++
 arch/arm/boot/dts/armada-xp-mv78460.dtsi |  112 ++++++++++++++++++++++++++++++
 3 files changed, 246 insertions(+)

index c45c7b4..a4c4fca 100644
@@ -65,5 +65,67 @@
 			#interrupts-cells = <2>;
 			interrupts = <20>, <21>, <22>;
 		};
+
+		/*
+		 * MV78230 has 2 PCIe units Gen2.0: One unit can be
+		 * configured as x4 or quad x1 lanes. One unit is
+		 * x4/x1.
+		 */
+		pcie-controller {
+			compatible = "marvell,armada-370-xp-pcie";
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0       0xd0040000 0x2000 /* port0x1_port0 */
+				  0x2000  0xd0042000 0x2000 /* port2x1_port0 */
+				  0x4000  0xd0044000 0x2000 /* port0x1_port1 */
+				  0x8000  0xd0048000 0x2000 /* port0x1_port2 */
+				  0xC000  0xd004C000 0x2000 /* port0x1_port3 */>;
+
+			pcie0.0 at 0xd0040000 {
+				reg = <0x0 0x2000>;
+				interrupts = <58>;
+				clocks = <&gateclk 5>;
+				marvell,pcie-port = <0>;
+				marvell,pcie-lane = <0>;
+				status = "disabled";
+			};
+
+			pcie0.1 at 0xd0044000 {
+				reg = <0x4000 0x2000>;
+				interrupts = <59>;
+				clocks = <&gateclk 5>;
+				marvell,pcie-port = <0>;
+				marvell,pcie-lane = <1>;
+				status = "disabled";
+			};
+
+			pcie0.2 at 0xd0048000 {
+				reg = <0x8000 0x2000>;
+				interrupts = <60>;
+				clocks = <&gateclk 5>;
+				marvell,pcie-port = <0>;
+				marvell,pcie-lane = <2>;
+				status = "disabled";
+			};
+
+			pcie0.3 at 0xd004C000 {
+				reg = <0xC000 0x2000>;
+				interrupts = <61>;
+				clocks = <&gateclk 5>;
+				marvell,pcie-port = <0>;
+				marvell,pcie-lane = <3>;
+				status = "disabled";
+			};
+
+			pcie2 at 0xd0042000 {
+				reg = <0x2000 0x2000>;
+				interrupts = <99>;
+				clocks = <&gateclk 7>;
+				marvell,pcie-port = <2>;
+				marvell,pcie-lane = <0>;
+				status = "disabled";
+			};
+		};
 	};
 };

1.7.9.5

The PlatHome OpenBlocks AX3-4 has an internal mini-PCIe slot that can be used to plug mini-PCIe devices. We therefore enable the PCIe interface that corresponds to this slot.

---
 arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts |    7 +++++++
 1 file changed, 7 insertions(+)

index b42652f..d9912df 100644
@@ -121,5 +121,12 @@
 			nr-ports = <2>;
 			status = "okay";
 		};
+		pcie-controller {
+			status = "okay";
+			/* Internal mini-PCIe connector */
+			pcie0.0 at 0xd0040000 {
+				status = "okay";
+			};
+		};
 	};
 };

1.7.9.5

The Marvell evaluation board (DB) for the Armada XP SoC has 6 physicals full-size PCIe slots, so we enable the corresponding PCIe interfaces in the Device Tree.

---
 arch/arm/boot/dts/armada-xp-db.dts |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

index 8e53b25..cd378ae 100644
@@ -90,5 +90,32 @@
 			phy = <&phy3>;
 			phy-mode = "sgmii";
 		};
+
+		pcie-controller {
+			status = "okay";
+
+			/*
+			 * All 6 slots are physically present as
+			 * standard PCIe slots on the board.
+			 */
+			pcie0.0 at 0xd0040000 {
+				status = "okay";
+			};
+			pcie0.1 at 0xd0044000 {
+				status = "okay";
+			};
+			pcie0.2 at 0xd0048000 {
+				status = "okay";
+			};
+			pcie0.3 at 0xd004C000 {
+				status = "okay";
+			};
+			pcie2 at 0xd0042000 {
+				status = "okay";
+			};
+			pcie3 at 0xd0082000 {
+				status = "okay";
+			};
+		};
 	};
 };

1.7.9.5

The Globalscale Mirabox platform uses one PCIe interface for an available mini-PCIe slot, and the other PCIe interface for an internal USB 3.0 controller. We add the necessary Device Tree informations to enable those two interfaces.

---
 arch/arm/boot/dts/armada-370-mirabox.dts |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

index 3b40713..6de4b74 100644
@@ -52,5 +52,19 @@
 			phy = <&phy1>;
 			phy-mode = "rgmii-id";
 		};
+
+		pcie-controller {
+			status = "okay";
+
+			/* Internal mini-PCIe connector */
+			pcie0 at 0xd0040000 {
+				status = "okay";
+			};
+
+			/* Connected on the PCB to a USB 3.0 XHCI controller */
+			pcie1 at 0xd0080000 {
+				status = "okay";
+			};
+		};
 	};
 };

1.7.9.5

The Marvell evaluation board (DB) for the Armada 370 SoC has 2 physical full-size PCIe slots, so we enable the corresponding PCIe interfaces in the Device Tree.

---
 arch/arm/boot/dts/armada-370-db.dts |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

index 0004402..56f2c14 100644
@@ -59,5 +59,20 @@
 			phy = <&phy1>;
 			phy-mode = "rgmii-id";
 		};
+
+		pcie-controller {
+			status = "okay";
+			/*
+			 * The two PCIe units are accessible through
+			 * both standard PCIe slots and mini-PCIe
+			 * slots on the board.
+			 */
+			pcie0 at 0xd0040000 {
+				status = "okay";
+			};
+			pcie1 at 0xd0080000 {
+				status = "okay";
+			};
+		};
 	};
 };

1.7.9.5

Now that we have the necessary drivers and Device Tree informations to support PCIe on Armada 370 and Armada XP, enable the CONFIG_PCI option.

Also, since the Armada 370 Mirabox has a built-in USB XHCI controller connected on the PCIe bus, enable the corresponding options as well.

---
 arch/arm/configs/mvebu_defconfig |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index a702fb3..cf597db 100644
@@ -13,6 +13,7 @@ CONFIG_MACH_ARMADA_370=y
 CONFIG_MACH_ARMADA_XP=y
 # CONFIG_CACHE_L2X0 is not set
 # CONFIG_SWP_EMULATE is not set
+CONFIG_PCI=y
 CONFIG_SMP=y
 # CONFIG_LOCAL_TIMERS is not set
 CONFIG_AEABI=y

@@ -38,7 +39,8 @@ CONFIG_I2C=y
 CONFIG_I2C_MV64XXX=y
 CONFIG_GPIOLIB=y
 CONFIG_GPIO_SYSFS=y
-# CONFIG_USB_SUPPORT is not set
+CONFIG_USB=y
+CONFIG_USB_XHCI_HCD=y
 CONFIG_RTC_CLASS=y
 CONFIG_RTC_DRV_S35390A=y
 CONFIG_DMADEVICES=y

1.7.9.5

I think a sane way to address this is to model the SOC as the root of the PCI-E and then model each of the ports as a non-compliant PCI-E bridge. The internal memory windows functionally map exactly to a PCI-E bridge memory window - just with annoyingly different register settings.

In this way you'd assign physical regions to the SOC and the linux discovery code would allocate and assign resources as appropriate, including the simulated bridges.

How exactly to cleanly hook the PCI code to simulate a bridge, I'm not sure, but something like this re-uses the bulk of the existing code to solve this problem.

Frankly, Marvell should have designed their cores to present this way via PCI-E config transactions :|

Not sure how big a deal this is though, my kirkwood systems that use PCI-E have a DT that exactly matches the resource assignment I expect from the kernel, so there is no wasted physical space. This is fine for embedded with no pluggable PCI-E..

I would be able to test a combined driver on Kirkwood, drop me a note if you do this please :)

Regards,
Jason

Mainly as background:

I /think/ Tegra has a similar HW setup (but perhaps not identical) (based on a very brief reading of your emails and brief knowledge of this aspect of the Tegra HW).

On Tegra, there is a 1GB physical address window that the PCIe controller serves. The controller has 2 or 3 ports, each a separate PCIe domain I believe. There are registers in the PCIe controller which route accessed made to the 1GB physical window to the various child ports and transaction types.

IIRC, the bindings Thierry came up with for the Tegra PCIe controller statically describe the setup of those mappings (e.g. it could assign a 256MB physical address region to port 1, and a 768MB physical address region to port 2 perhaps?).

It sounds like Jason is advocating a much more dynamic approach on the Marvell HW. Perhaps this is related to whether the n host ports driven by the controller are separate PCIe domains (as I believe they are or can be on Tegra) or not.

Dear Stephen Warren,

On Marvell SoCs, this is even more flexible: you have 20 configurable address decoding windows. For each of them, you can configure the base address, size, and target device (i.e PCIe port x.y, NAND, or some other devices). And since we have up to 10 PCIe interfaces, we really don't want to over-allocate hundreds of MB of physical address space for each device, since most of them need only a few dozens of KB.

The code I'm proposing for the Marvell PCIe is already dynamic it doesn't describe the addresses in the DT, but instead dynamically allocates address decoding windows according to the number of PCIe devices that are found.

However, I haven't found yet how to get, during the ARM pcibios initialization, the memory size needed for each device, so that I could size the address decoding windows correctly instead of over-allocating them (currently I allocate SZ_64MB for each window, even for my e1000e PCIe card that needs only SZ_32K of PCI memory space).

Best regards,

Thomas

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

OK, that all makes sense.

One question though: When you say "device" in the line above, I assume the device you're referring to is the PCIe host device, and not the individual PCIe devices themselves; with 20 address decoding windows and 10 PCIe ports, and those windows apparently being used for on-SoC devices too (e.g. you mention NAND above), I assume you'd want to limit the number of windows you use per PCIe bus/port to just 1, rather than 1 per enumerated PCIe device?

Dear Stephen Warren,

Well, I am not a PCI or PCIe expert, so my terminology might be wrong. Basically, on Armada XP, you have:

PCIe 0.0 (can be x4, in which case PCIe 0.{1,2,3} can't be used, or x1) PCIe 0.1 x1
PCIe 0.2 x1
PCIe 0.3 x1
PCIe 1.0 (can be x4, in which case PCIe 1.{1,2,3} can't be used, or x1) PCIe 1.1 x1
PCIe 1.2 x1
PCIe 1.3 x1
PCIe 2.0 (x4/x1)
PCIe 3.0 (x4/x1)

On the Armada XP evaluation board, I have one slot for each of those interfaces (well, actually I don't have slots for the PCIe1.{0,1,2,3}), so I can connect only one PCIe device (i.e a NIC card over PCIe, or a SATA card over PCIe, or a USB controller over PCIe) per PCIe interface.

And the address decoding windows are associated to a PCIe interface (through its x.y number). So for now, there is a one-to-one mapping between a PCIe interface and a PCIe device, and therefore with an address decoding window.

That said, I suppose that what you're thinking of are PCIe bridges, is that correct? So those would allow to connect multiple PCIe devices on a single PCIe interface (for example PCIe 3.0 listed above). In that case, I suppose my address decoding window would have to have a size greater than or equal to the sum of the size of all BARs of the PCIe devices found on the downstream bus. Lior, could you confirm or infirm my statement?

Does that answer your question?

Thomas

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

You are correct, the SOC decoding window would have to match the bridge configuration of the PCI-e bridge the port is hooked up to, and the bridge configuration will extend across all the BARs of its children devices. Note there are special alignment considerations for PCI-E bridge windows - the Linux code handles all this though.

To support PCIe bridges you also will need to allocate a bus number resource range to each port, along with the two memory and one IO resource ranges. That should be in device tree too .. The current Marvell drivers don't do this and the kernel complains:

pci_bus 0000:00: No busn resource found for root bus, will use [bus 00-ff]

Bus number ranges should be disjoint so we can avoid domains.

Jason

On 12/10/2012 11:59 AM, Thomas Petazzoni wrote:
...

...

Yes, I was definitely thinking of the device that's plugged into a PCIe port being a bridge itself, with multiple downstream devices.

That sounds about right.

Yes, thanks.

Fundamentally this is similar to what Marvell did, the routing registers are functionally identical to PCI-E bridges, but don't use PCI-E standard configuration.

Look at how Intel models their PCI-E and you can see the same functional elements:

00:00.0 Host bridge: Intel Corporation 2nd Generation Core Processor Family DRAM Controller (rev 09) 00:1c.0 PCI bridge: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 1 (rev b4) 00:1c.4 PCI bridge: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 5 (rev b4) 00:1c.6 PCI bridge: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 7 (rev b4) 00:1c.7 PCI bridge: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 8 (rev b4)

1c.x are *physical* PCI-E ports, while 00 represents the internal ring bus. All of the above devices are on the CPU SOC.

The address decoding windows of the physical ports are configured via standard PCI-E bridge configuration space accesses and control which addresses from the internal ring bus go out the port, as well as which bus number range the port handles. This the same function as the address decoding windows, just expressed via a standard register layout instead of as a SOC specific feature.

By providing actual PCI configuration space to control all this it is automatically compatible with standard PCI configuration code.

The mistake these SOCs are making is not using standard PCI modeling to describe their PCI functional blocks :|

What you'd want to see is the same arrangement as Intel:

00:00.0 Host bridge: SOC software emulated host bridge 00:01.1 PCI bridge: SOC software emulated PCI-E root port 1 00:01.2 PCI bridge: SOC software emulated PCI-E root port 2 00:02.0 Ethernet device..

With a tree like:
00:00.0 -> 0:01.1

          -> 0:01.2 -> 00:02.0

Discovery is started from 00:00.0 and flows through all the ports in one pass.

It is already the case we pretty much have a software emulated root port, at least on Marvell the config space returned for the PCI-E port is useless/wrong when in root port mode.

If you can't do full dynamic configuration, then I think at least the allocation size per port needs to be in device tree.

Any system with multiple pci-e ports and non-PCI-E configuration registers can be modeled like this.

It sounds like the concept of a PCI-E bridge with internal configuration could be generalized for more types of hardware that the Marvell case. Pretty much all socs will have a similar design, a number of PCI-E ports, a collection of address decoding/routing windows and some registers to control them, someplace...

Also note that a 'PCI domain' is a Linux concept, it refers to a PCI bus tree that has bus numbers that can overlap with other busses in the system. You only need this concept if you have more than 255 PCI busses, and have a way to route configuration cycles to a specific physical ports. It is generally much more understandable to just assign unique PCI-E bus numbers to every bus and forgo domains..

Jason

Dear Jason Gunthorpe,

Hum, ok, this makes sense. I have no idea at the moment how to achieve that, but I'll try to have a look. Do you have pointers to code doing this?

Indeed. But for example, in Marvell's case, the address decoding windows mechanism is not specific to PCIe, it is also used for other devices, so the management of those decoding windows cannot be entirely left to the PCIe driver.

Best regards,

Thomas

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

I haven't studied the Linux code specifically for this, but a quick perusal through the header file isn't showing up any existing support.

You'd have to confer with the PCI maintainers what they want, but a possible way to start would be to fake the configuration query results. This is already being done via a fixup to make the root port report as a host bridge.

Yes, though you might want to think about having the window numbers assigned staticly (for PCI-E and everything else) in device tree rather than all the dynamic code. It would be simpler to just run through the configuration that device tree has..

The window stuff is causing me problems today, I'm looking at an option to have to kernel not program any windows. The kernel destroys the settings from the bootloader, which are more correct than what it puts in - in practice this means my kexec flow doesn't work since I can't re-enter the bootload because the kernel unmapped it.

Jason

Dear Jason Gunthorpe,

So I should implement fake PCI configuration read/write operations, and emulate a PCIe bridge? Sounds complicated...

Definitely not. We have a maximum of 20 address decoding windows, for all devices. We have 10 PCIe interfaces, each might require 2 windows:
one for I/O BARs, one for memory BARs, that would make 20 windows, not leaving any windows for other devices. Ditto for the size: if we create 10 windows for the 10 memory BARs of the 10 PCIe interfaces, with a size of 128 MB, we "consume" 1.2 GB of physical address space, which we definitely don't want to do. Older Marvell SoCs (Kirkwood) were doing a static allocation of address decoding windows because there were only 2 PCIe interfaces. But now that there are 10 of them, this is clearly not possible.

Best regards,

Thomas

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Well, I can give you an outline what that would look like and you can think about it.

I'd suggest something like

struct pcie_sw_rp_ops

{
    int (*setup_port)(unsigned int portnum,..);
    int (*config_read)(unsigned int portnum,..);
    int (*config_write)(unsigned int portnum,..);
    int (*window_setup)(unsigned int portnum,..);
    int (*config_pcie_link_read)(unsgined int portnum,..);
    int (*config_pcie_link_write)(unsigned int portnum,..);
};

// This gets passed to pci_common_init or something more general struct pcie_sw_rp

{
    // pcie_sw_rp's code has a pcie_ops associated with this
    struct hw_pci pci; 
    unsigned int num_ports;
    const struct pcie_sw_rp_ops *ops
};

pcie_sw_rp models a soft root port, a low level driver creates one of these objects and supplies pcie_sw_rp_ops. pcie_sw_rp's code is the entry point from the pci stack, via it's pcie_ops. A sw_rp bundles num_ports worth of physical PCI-E ports together into a root complex that has a single host bridge and a PCI-E bridge for every port. It hides the PCI-E configuration space of the underlying hardware from the kernel because the hardware is not a compliant with PCI.

For all configuration operations
- If the target is 00:00.0 then return a static array of data representing a standard PCI-E host bridge. Discard all writes. This is easy
- For 00:0x10+N.0 where N is between 0 and num_ports
- Return static data for a bridge header
- Static bridge header, static secondary status, slightly dynamic bridge ctrl
- PCI-E Root Port capability block
- Static Master state and caps
- call ops->config_pcie_link_* for the slave caps
(you should be able to get a prototype working without the PCI-E
capability block)
- No need for MSI, power management/etc.
- Map the AER cap via ops (not needed for basic support)
- Capture and cache writes to the four bridge window registers (IO, MMIO, prefetch and busnumber)
- When the bridge window enable is set call ops->window_setup() on all four captured resource ranges. window_setup is expected to make it so CPU accesses to the given resource range appear on that port.

Most of the configuration block data can be a static array, only a little actually needs to be dynamic. You can review and copy the lspci dump from an Intel box to get this right.

I didn't check, but the alignment requirements of bridge configuration and what the HW can do will have to match. If this can't be done then some kind of fixup to the PCI-E configurator would be needed........

The purpose of all this is to correct the end-port focused PCI-E configuration space that Marvell and other SOC vendors use to have a correct root port configuration model. Hijacking the configuration IOs to do this is a bit ugly, but does present a correct and consistent view to userspace. Keeping it general will allow Samsung and others to use it as well.

Probably a few hundred lines all told. It isn't 'hard' but it will be a bit finicky..

The other approach would be to try and model all this directly via PCI-E structures, but there is no existing code support for that, and user space would see a very confusing view.

Right, that is the point.. If you are actually going to use all 10 PCI-E interfaces then the DT needs to control which interfaces get an IO mapping window and you need to keep the total # of allocated windows below 20. (perhaps by not mapping other units, like crypto or xor)

You can't rely on fully dynamic allocation for this because there is no way for the PCI layer to know if the IO or MMIO window is going to be used by the driver that attaches. In the vast majority of cases the MMIO window will be used and the IO is not necessary..

Jason

This sounds like something we could very much use on Tegra as well. It's the final piece of the puzzle if I understand correctly. Some entity that the root ports can hang off of.

I agree, the configuration IO accessors seem to be the only way to get the kernel and userspace to see the same view.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121216/22d4f2df/attachment.sig>

No, the ports on Tegra are not separate PCIe domains. The configuration space mapping is shared between all ports and is programmed in the register space of the PCIe controller. This is what PCIe refers to as ECAM, only with a slightly incompatible mapping.

The registers that I think you're referring to allow the type of access for a given region (I/O, prefetchable and non-prefetchable MMIO) to be specified. They cannot be programmed to route accesses to a specific port. The most recent bit of information was that this is derived from the standard PCI registers that are available for each port.

Yes, that's correct. I also have experimental patches that add a bus-range property to be assigned to each port, but that doesn't quite work yet. The PCI core keeps complaining about the assigned bus range conflicting with [0x00-0xff], which AFAICT is the one assigned by default if no bus-range was specified by the driver.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121211/6a8de55c/attachment-0001.sig>

OK, so can you please remind me why the top-level PCIe controller node has a child node for each port, with hard-coded non-intersecting ranges for configuration space access? If they all go through the same address range, and use standard PCI bridge registers to route transactions to the separate ports, I would have expected no need for explicit per-port sub-nodes or static address allocations.

The ranges for configuration space access are phony. I think that's something that Mitch suggested at the time because you need to pass ranges such as this to the children somehow. Within the driver only the configuration space as specified in the pcie-controller node's reg property is used.

As for the separate child nodes, those are still required to model each port as a separate bus. I don't see how this could be done any other way, but maybe that's not what you were asking.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121212/73e7d23e/attachment.sig>

...

If the PCIe controller acts like a PCIe bridge, then won't the separate ports show up on the PCIe bus and hence be auto-probed and hence not require and representation in the DT at all? Or, does the HW simply use standardized registers to program the routing of transactions to the ports, yet not actually appear on the PCIe bus as a bridge device?

I don't think that's the way it works. The PCIe controller is what the PCIe specification refers to as root complex. The root complex can have one or more root ports. In case of Tegra20 that would be 2 root ports, Tegra30 has 3. Each of these root ports is a virtual PCI-PCI bridge within the root complex. However, they don't "appear" on the bus since they are the origins of two busses.

At least that's how I read the downstream code and how the upstream code works as well. For each port a new pci_bus is created and enumerated. I think the problem that prevents it from working the way you proposed is that each root port has a special way of accessing the configuration space, which is a region that is mapped to the register space of each root port. If you look at the tegra_pcie_{read,write}_conf() functions you'll see how they are special-cased for the root ports.

Perhaps it will even work the way you suggested if instead we removed the special-case for root ports and enumerate only one root bus. I'll need to check that.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121213/b00e27cf/attachment.sig>

PCIe is very specific about what a root complex is:

Root Complex =
An entity that includes a Host Bridge, zero or more Root Complex Integrated Endpoints, zero or more Root Complex Event Collectors, and one or more Root Ports.

It also has other specific text requiring that each of the root complex items conform to the software configuration interface - they must be discoverable, they must have device numbers, they must have configuration space.

Stephen is correct in how things should work - the physical PCI-E link off the SOC should be reporting to linux as the secondary port on a PCIe-PCIe bridge, and discovery should naturally follow along it from the host bridge with no additional configuration or DT modeling required.

When linux starts enumeration the bus 0, device 0, function 0 configuration space should be a host bridge. Bus 0 devices XX should be the virtual PCI-PCI bridges.

This is not compliant.

When the spec talks about a 'virtual PCI-PCI bridge' it means virtual in the sense that it is not seperate physical hardware. It absolutely must appear in PCI device enumeration.

The spec is very clear on this point:

bridge =
A device that virtually or actually connects a PCI/PCI-X segment or PCI Express Port with an internal component interconnect or with another PCI/PCI-X segment or PCI Express Port. A virtual Bridge in a Root Complex or Switch must use the software configuration interface described in this specification.

You might be missing the host bridge. If the SOC is providing compliant PCIe bridges with configuration space for the root ports but does not provide the host bridge configuration space then you may need to provide the host bridge in software to tie it all together into a compliant root complex.

Regards,
Jason

Okay, so I gather that all of the above means that *if* the Tegra PCIe controller is compliant, it should just work if we remove any of the special cases. I'm not sure if anybody's actually tested this or if it just isn't compliant. I'll see if I can find some time to test this. Obviously it would be a whole lot nicer if the hardware really was compliant so that we don't have to emulate the host bridge in software.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121213/20939022/attachment.sig>

On 12/13/2012 01:23 AM, Thierry Reding wrote:
...

A little background here:

IIUC, the Tegra PCIe controller is at least partially derived from previous MCP projects, which I /assume/ must have been fully compliant since they were standard x86 HW. I'm basing the derivation comment on the fact that certain aspects of the HW are the same as previous MCP designs apparently: the configuration space access register fields, and the physical (internal bus) addresses you write into PCIe controller's memory windows.

So, this implies it's entirely possible that the PCIe controller is in fact fully compliant.

That all said, it's quite possible that the parts which made it compliant were stripped out when taking the IP block out of MCP (where the upstream bus was probably HyperTransport?) and dumping it into Tegra, which required different logic to interface to the upstream bus.

Hmmm. I guess all this still means that you just need to try it and see. If you need me to try to track down any answers about the HW, let me know.

So I tried this today and it breaks horribly. There's some internal abort or something. I don't have access to the hardware right now and forgot to save the log output, but I can follow up in the morning. Also up until the abort, bus 0000:00.0 was identified as the virtual switch within the FPGA that's connected to port 0, so that would indicate that it isn't in fact compliant and neither root port is reachable via the regular mapping.

I suppose that may be the reason why the downstream code implements the special case for accesses to the root ports' configuration space.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121213/638cef9d/attachment-0001.sig>

With the special case, what does device 0:0.0 show up as? What class?

Jason

I think that's the root port, with a bridge class. But again, I can follow up with the full output of lspci tomorrow morning.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121213/0b100d95/attachment.sig>

So here's some more output:

-sh-4.2# lspci
00:00.0 PCI bridge: NVIDIA Corporation Device 0bf0 (rev a0)
01:00.0 PCI bridge: PLD APPLICATIONS Device 4711
02:00.0 PCI bridge: Avionic Design GmbH FPGA PCIe PCI-to-PCI (P2P) Bridge 02:01.0 PCI bridge: Avionic Design GmbH FPGA PCIe PCI-to-PCI (P2P) Bridge 02:02.0 PCI bridge: Avionic Design GmbH FPGA PCIe PCI-to-PCI (P2P) Bridge 02:03.0 PCI bridge: Avionic Design GmbH FPGA PCIe PCI-to-PCI (P2P) Bridge 02:04.0 PCI bridge: Avionic Design GmbH FPGA PCIe PCI-to-PCI (P2P) Bridge 02:05.0 PCI bridge: Avionic Design GmbH FPGA PCIe PCI-to-PCI (P2P) Bridge 02:06.0 PCI bridge: Avionic Design GmbH FPGA PCIe PCI-to-PCI (P2P) Bridge 02:07.0 PCI bridge: Avionic Design GmbH FPGA PCIe PCI-to-PCI (P2P) Bridge 03:00.0 Memory controller: Avionic Design GmbH FPGA PCIe Test Endpoint 04:00.0 Serial bus controller [0c80]: Avionic Design GmbH OpenCores SPI Controller 05:00.0 Serial bus controller [0c80]: Avionic Design GmbH OpenCores I2C Controller 06:00.0 Intelligent controller [0e80]: Avionic Design GmbH 64 pin GPIO Controller 07:00.0 Modem: Avionic Design GmbH OpenCores 16550 UART
08:00.0 Modem: Avionic Design GmbH OpenCores 10/100 Mbps Ethernet Controller 09:00.0 Modem: Avionic Design GmbH OpenCores CAN Protocol Controller 0a:00.0 Modem: Avionic Design GmbH OpenCores CAN Protocol Controlle

-sh-4.2# lspci -tv
-[0000:00]---00.0-[01-0a]----00.0-[02-0a]--+-00.0-[03]----00.0 Avionic Design GmbH FPGA PCIe Test Endpoint
+-01.0-[04]----00.0 Avionic Design GmbH OpenCores SPI Controller +-02.0-[05]----00.0 Avionic Design GmbH OpenCores I2C Controller +-03.0-[06]----00.0 Avionic Design GmbH 64 pin GPIO Controller +-04.0-[07]----00.0 Avionic Design GmbH OpenCores 16550 UART +-05.0-[08]----00.0 Avionic Design GmbH OpenCores 10/100 Mbps Ethernet Controller +-06.0-[09]----00.0 Avionic Design GmbH OpenCores CAN Protocol Controller \-07.0-[0a]----00.0 Avionic Design GmbH OpenCores CAN Protocol Controller

-sh-4.2# lspci -s 00:00.0 -v
00:00.0 PCI bridge: NVIDIA Corporation Device 0bf0 (rev a0) (prog-if 00 [Normal decode])
Flags: bus master, fast devsel, latency 0
Bus: primary=00, secondary=01, subordinate=0a, sec-latency=0 Prefetchable memory behind bridge: 00000000b0000000-00000000b0ffffff
Capabilities: [40] Subsystem: NVIDIA Corporation Device 0000
Capabilities: [48] Power Management version 3
Capabilities: [50] MSI: Enable- Count=1/2 Maskable- 64bit+
Capabilities: [60] HyperTransport: MSI Mapping Enable- Fixed-
Capabilities: [80] Express Root Port (Slot+), MSI 00
Capabilities: [100] #00

So the class is 0x0604. I unfortunately have no setup where hardware is connected to the second port. It would be interesting to see how that looks.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121214/472414a8/attachment-0001.sig>

So here's the output of the crash when removing the special cases that I promised:

[    2.662948] tegra-pcie 80003000.pcie-controller: PCI host bridge to bus 0000:00
[    2.670271] pci_bus 0000:00: root bus resource [io  0x82000000-0x8200ffff]
[    2.687624] pci_bus 0000:00: root bus resource [mem 0x81000000-0xa7ffffff]
[    2.696002] pci_bus 0000:00: root bus resource [mem 0xb0000000-0xb7ffffff pref]
[    2.708361] pci_bus 0000:00: root bus resource [bus 00-ff]
[    2.746728] pci 0000:00:00.0: [1556:4711] type 01 class 0x060400
[    2.754366] pci 0000:00:00.0: PME# supported from D0 D3hot D3cold
[    2.764883] Unhandled fault: imprecise external abort (0x406) at 0x000492f4
[    2.771845] Internal error: : 406 [#1] PREEMPT SMP ARM
[    2.776976] Modules linked in:
[    2.780040] CPU: 0    Not tainted  (3.7.0-next-20121213-00089-gc99c049-dirty #26)
[    2.787522] PC is at tegra_pcie_read_conf+0x50/0xa4
[    2.792407] LR is at pci_bus_read_config_dword+0x68/0x88
[    2.797723] pc : [<c0024974>]    lr : [<c02055e0>]    psr: 80000093
[    2.797723] sp : da8a3d08  ip : e1000000  fp : c06a9064
[    2.809193] r10: 00000000  r9 : da8a3de0  r8 : dab8d800
[    2.814413] r7 : 00000000  r6 : 00000000  r5 : 00000000  r4 : 00000800
[    2.820931] r3 : 00000004  r2 : 00000000  r1 : e1000800  r0 : ffffffff
[    2.827451] Flags: Nzcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
[    2.834836] Control: 10c5387d  Table: 1a10804a  DAC: 00000015
[    2.840574] Process kworker/u:3 (pid: 55, stack limit = 0xda8a2238)
[    2.846833] Stack: (0xda8a3d08 to 0xda8a4000)
[    2.851191] 3d00:                   dab8d800 da8a3d6c 60000013 00000008 da8a3d2c c020a920
[    2.859362] 3d20: 00000000 00000008 dae99c00 00000000 00000000 da8a3d6c 0000ea60 c0206c10
[    2.867533] 3d40: c06ae404 00000008 00000000 dab8d800 00000008 00000000 00000000 da8a3de0
[    2.875704] 3d60: 00000000 c04a0538 fee00000 47111556 beffffff 00000010 dab8d800 00000008
[    2.883875] 3d80: 00000000 c0207fcc 00000010 dab8d800 da8dd790 00000000 00000000 c0208a5c
[    2.892047] 3da0: dab8d800 dab8e840 da8dd790 00000000 00000000 c002535c dab8e860 dab8e840
[    2.900218] 3dc0: dab8e840 dab8e860 da8a3e3c c001309c 00000000 da82e8d0 da8dd790 00000083
[    2.908389] 3de0: da8a3de0 da8a3de0 db4a3e10 00000000 db4a3e10 da8dd790 00000000 dae0c150
[    2.916560] 3e00: c0690434 dae0c350 0000008c c00264bc 00000000 c059f6f8 da8dd790 c011ce0c
[    2.924732] 3e20: dae0c2b4 00000034 da8dc990 00000001 db4a3e10 da8dd830 da8dd790 00000000
[    2.932902] 3e40: 00000001 da8a3e38 c0025398 c00252e0 00000000 00000000 00000000 c0024ac8
[    2.941074] 3e60: db4a3e10 c0714d08 db4a3e10 c024de98 c06a3ef4 00000000 dae0ee00 c024d654
[    2.949245] 3e80: c06f41cc c024eeb0 c024ee98 c024dc70 00000000 00000000 db4a3e10 c024de98
[    2.957415] 3ea0: 00000000 c024c3e0 db46faf4 db5b9df8 db4a3e10 db4a3e44 00000000 c024dbc4
[    2.965587] 3ec0: db46fa80 db4a3e10 db4a3e10 c06b7ce8 00000000 c024d248 db4a3e10 c06b7c80
[    2.973758] 3ee0: c06b7c64 c024d6b4 c06b7c9c da7a5680 c06f40c0 c003fd8c 00000001 db481ea8
[    2.981930] 3f00: 00000000 00000000 c0051880 c06f41cc da7a5690 da8a2000 c06f40c0 c0691510
[    2.990102] 3f20: 677d46cf c06f4080 da7a5680 c00403d4 c004026c da8a2000 c0686c00 c0686c00
[    2.998273] 3f40: c004026c db481e90 00000000 da7a5680 c004026c 00000000 00000000 00000000
[    3.006443] 3f60: 00000000 c00445cc 6b6b6b6b 00000000 6b6b6b6b da7a5680 00000000 00000000
[    3.014615] 3f80: da8a3f80 da8a3f80 00000000 00000000 da8a3f90 da8a3f90 da8a3fac db481e90
[    3.022785] 3fa0: c0044528 00000000 00000000 c000e418 00000000 00000000 00000000 00000000
[    3.030955] 3fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    3.039126] 3fe0: 00000000 00000000 00000000 00000000 00000013 00000000 6b6b6b6b a56b6b6b
[    3.047309] [<c0024974>] (tegra_pcie_read_conf+0x50/0xa4) from [<da8a3d2c>] (0xda8a3d2c)
[    3.055395] Code: e0811000 e08c1001 e5910000 f57ff04f (e3530001) 
[    3.061484] ---[ end trace 218fc1c8f4f9d245 ]---
[    3.066098] note: kworker/u:3[55] exited with preempt_count 1
[    3.071966] tegra-pcie 80003000.pcie-controller: AXI response decoding error, signature: ff000801
[    3.081021] Unable to handle kernel paging request at virtual address ffffffec
[    3.088239] pgd = c0004000
[    3.090945] [ffffffec] *pgd=1ffdd821, *pte=00000000, *ppte=00000000
[    3.097234] Internal error: Oops: 17 [#2] PREEMPT SMP ARM
[    3.102622] Modules linked in:
[    3.105685] CPU: 0    Tainted: G      D       (3.7.0-next-20121213-00089-gc99c049-dirty #26)
[    3.114132] PC is at kthread_data+0x4/0xc
[    3.118142] LR is at wq_worker_sleeping+0xc/0xe4
[    3.122758] pc : [<c0044900>]    lr : [<c0040784>]    psr: 00000193
[    3.122758] sp : da8a3aa8  ip : 00000000  fp : da8a3b5c
[    3.134218] r10: da8a2000  r9 : c0686c40  r8 : da8e4814
[    3.139436] r7 : c0b2bc40  r6 : 00000000  r5 : 00000000  r4 : da8e4890
[    3.145953] r3 : 00000000  r2 : 00000000  r1 : 00000000  r0 : da8e45c0
[    3.152472] Flags: nzcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
[    3.159684] Control: 10c5387d  Table: 1a10804a  DAC: 00000015
[    3.165421] Process kworker/u:3 (pid: 55, stack limit = 0xda8a2238)
[    3.171679] Stack: (0xda8a3aa8 to 0xda8a4000)
[    3.176036] 3aa0:                   da8e4890 00000000 da8e45c0 c04ab0d4 db450e10 db41c990
[    3.184208] 3ac0: db404900 c00c5694 35040002 00000035 00000002 c0686c40 c0686c40 c0686c40
[    3.192379] 3ae0: c0686c40 c0686c40 00000000 c0685700 c0685700 c00c58cc da8e45c0 dab91280
[    3.200550] 3b00: 00000002 da8e45c0 c0685700 c002d000 da8a3b4c c000e000 c068a040 c0685700
[    3.208722] 3b20: 00000002 c0b2afd8 da8e4814 c0084860 da8e45c0 00000000 da8e4814 00000001
[    3.216893] 3b40: da8e45b8 da8e45c0 da8e45b8 da8e4814 db44ba40 c0024976 da8e4814 c002e23c
[    3.225064] 3b60: 60000193 da8a3b78 da8a2000 00000001 c0024978 da8e485c 00520052 da8a3b78
[    3.233236] 3b80: da8a2000 c06d11d8 da8a2000 c0694ae8 60000193 0000000b 00000001 c0024976
[    3.241407] 3ba0: c0024978 c0011a98 da8a2238 0000000b c06d273e da8a2000 00000000 00000008
[    3.249579] 3bc0: 65000000 31313830 20303030 63383065 31303031 39356520 30303031 35662030
[    3.257751] 3be0: 30666637 28206634 33353365 31303030 c0002029 c04a5c70 c059cb74 00000406
[    3.265922] 3c00: 00000007 c0695668 000492f4 da8a3cc0 da8a3de0 00000000 c06a9064 c0008604
[    3.274093] 3c20: 00000000 c0686c40 00000007 00000000 00030003 000492f4 c0b2bc40 c0686c40
[    3.282264] 3c40: fe000100 00000000 da8a3c5c c004f5bc c0b2bc40 da8a2000 c0686514 00000000
[    3.290435] 3c60: fe000100 00000000 00000000 c0030308 00000000 c00138a4 fe00010c c0690458
[    3.298607] 3c80: da8a3ca0 c0008700 c0205680 c04ac168 60000013 ffffffff da8a3cd4 c000e000
[    3.306778] 3ca0: c0712fb8 60000013 c0024974 80000093 ffffffff da8a3cf4 dab8d800 c000df98
[    3.314949] 3cc0: ffffffff e1000800 00000000 00000004 00000800 00000000 00000000 00000000
[    3.323120] 3ce0: dab8d800 da8a3de0 00000000 c06a9064 e1000000 da8a3d08 c02055e0 c0024974
[    3.331292] 3d00: 80000093 ffffffff dab8d800 da8a3d6c 60000013 00000008 da8a3d2c c020a920
[    3.339463] 3d20: 00000000 00000008 dae99c00 00000000 00000000 da8a3d6c 0000ea60 c0206c10
[    3.347633] 3d40: c06ae404 00000008 00000000 dab8d800 00000008 00000000 00000000 da8a3de0
[    3.355804] 3d60: 00000000 c04a0538 fee00000 47111556 beffffff 00000010 dab8d800 00000008
[    3.363975] 3d80: 00000000 c0207fcc 00000010 dab8d800 da8dd790 00000000 00000000 c0208a5c
[    3.372146] 3da0: dab8d800 dab8e840 da8dd790 00000000 00000000 c002535c dab8e860 dab8e840
[    3.380317] 3dc0: dab8e840 dab8e860 da8a3e3c c001309c 00000000 da82e8d0 da8dd790 00000083
[    3.388488] 3de0: da8a3de0 da8a3de0 db4a3e10 00000000 db4a3e10 da8dd790 00000000 dae0c150
[    3.396660] 3e00: c0690434 dae0c350 0000008c c00264bc 00000000 c059f6f8 da8dd790 c011ce0c
[    3.404830] 3e20: dae0c2b4 00000034 da8dc990 00000001 db4a3e10 da8dd830 da8dd790 00000000
[    3.413001] 3e40: 00000001 da8a3e38 c0025398 c00252e0 00000000 00000000 00000000 c0024ac8
[    3.421173] 3e60: db4a3e10 c0714d08 db4a3e10 c024de98 c06a3ef4 00000000 dae0ee00 c024d654
[    3.429345] 3e80: c06f41cc c024eeb0 c024ee98 c024dc70 00000000 00000000 db4a3e10 c024de98
[    3.437516] 3ea0: 00000000 c024c3e0 db46faf4 db5b9df8 db4a3e10 db4a3e44 00000000 c024dbc4
[    3.445688] 3ec0: db46fa80 db4a3e10 db4a3e10 c06b7ce8 00000000 c024d248 db4a3e10 c06b7c80
[    3.453860] 3ee0: c06b7c64 c024d6b4 c06b7c9c da7a5680 c06f40c0 c003fd8c 00000001 db481ea8
[    3.462030] 3f00: 00000000 00000000 c0051880 c06f41cc da7a5690 da8a2000 c06f40c0 c0691510
[    3.470203] 3f20: 677d46cf c06f4080 da7a5680 c00403d4 c004026c da8a2000 c0686c00 c0686c00
[    3.478373] 3f40: c004026c db481e90 00000000 da7a5680 c004026c 00000000 00000000 00000000
[    3.486544] 3f60: 00000000 c00445cc 6b6b6b6b 00000000 6b6b6b6b da7a5680 00000000 00000000
[    3.494715] 3f80: da8a3f80 da8a3f80 00000001 00010001 da8a3f90 da8a3f90 da8a3fac db481e90
[    3.502886] 3fa0: c0044528 00000000 00000000 c000e418 00000000 00000000 00000000 00000000
[    3.511056] 3fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    3.519227] 3fe0: 00000000 00000000 00000000 00000000 00000013 00000000 6b6b6b6b a56b6b6b
[    3.527412] [<c0044900>] (kthread_data+0x4/0xc) from [<c0040784>] (wq_worker_sleeping+0xc/0xe4)
[    3.536114] [<c0040784>] (wq_worker_sleeping+0xc/0xe4) from [<c04ab0d4>] (__schedule+0x4d4/0x6b0)
[    3.544995] [<c04ab0d4>] (__schedule+0x4d4/0x6b0) from [<c002e23c>] (do_exit+0x578/0x834)
[    3.553186] [<c002e23c>] (do_exit+0x578/0x834) from [<c0011a98>] (die+0x1e0/0x398)
[    3.560758] [<c0011a98>] (die+0x1e0/0x398) from [<c0008604>] (do_DataAbort+0x88/0x98)
[    3.568586] [<c0008604>] (do_DataAbort+0x88/0x98) from [<c000df98>] (__dabt_svc+0x38/0x60)
[    3.576836] Exception stack(0xda8a3cc0 to 0xda8a3d08)
[    3.581887] 3cc0: ffffffff e1000800 00000000 00000004 00000800 00000000 00000000 00000000
[    3.590059] 3ce0: dab8d800 da8a3de0 00000000 c06a9064 e1000000 da8a3d08 c02055e0 c0024974
[    3.598223] 3d00: 80000093 ffffffff
[    3.601718] [<c000df98>] (__dabt_svc+0x38/0x60) from [<c0024974>] (tegra_pcie_read_conf+0x50/0xa4)
[    3.610673] [<c0024974>] (tegra_pcie_read_conf+0x50/0xa4) from [<da8a3d2c>] (0xda8a3d2c)
[    3.618758] Code: e513001c e7e00150 e12fff1e e59032a4 (e5130014) 
[    3.624847] ---[ end trace 218fc1c8f4f9d246 ]---
[    3.629456] Fixing recursive fault but reboot is needed!

That doesn't look good, does it?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121214/2fe0b2fe/attachment.sig>

This is your

02:00.0 PCI bridge: Avionic Design GmbH FPGA PCIe PCI-to-PCI (P2P) Bridge

Device, right?

Just looking at the driver a bit, and your results, it looks to me like the config space for the internal devices is seperate from the register to send config packets to the bus(es).

So, it looks like what I suggested earlier is the trouble, you are missing the host bridge configuration

If you change tegra_pcie_read/write_conf to be more like:

static int tegra_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
int where, int size, u32 *val)

{
	/* Check the host bridge bus, all the pci-pci bridge ports
	   live here */
	if (bus->number == 0) {
           if (PCI_SLOT(devfn) >= 0x10 &&
	       PCI_SLOT(devfn) < 0x10 + tegra_pcie.num_ports &&
	       PCI_FUNC(devfn) == 0) {
               addr = tegra_pcie.port[PCI_SLOT(devfn) - 0x10].base + (where & ~0x3);
           } else {
	       *val = 0xffffffff;
               return PCIBIOS_DEVICE_NOT_FOUND;
           }
       }
}

Ie route access for 00:1N.0 to the configuration space on bridge port N

Also, you need to change the PCI core binding to report as only one controller, and probably some other minor fixups related to that.

Then you are a bit closer. You should see both root port bridges appear in your lspci.. IIRC the host bridge device is not essential to discovery working on Linux.

00:00.0 PCI bridge: NVIDIA Corporation Device 0bf0 (rev a0) (prog-if 00 [Normal decode])
Capabilities: [60] HyperTransport: MSI Mapping Enable- Fixed-

Heh, I wonder WTF that does on a ARM system! On a HT system that takes care of mapping PCIe format MSI to HT format MSI..

Jason

No, the 1556:4711 is the PLDA bridge. Basically that's the top-entity in the FPGA that handles the link layer from the Tegra SoC to the FPGA.

Is there any particular reason why you choose 0x10 as the base slot for bridge ports? With the latest DT support patches, mapping this should be even simpler as we associate a port index with each port and the port array is gone.

Yes, that's precisely what I've been doing. I currently have an ugly TEGRA_PCIE_SINGLE_BUS define that allows me to switch from a one bus per bridge port to one bus per controller configuration. I think the above is the missing point to make the latter configuration work. If I am not mistaken it should also solve a different issue that I've been seeing regarding the OF node to PCI device matching, since the busses as instantiated in the current implementation are root busses without a device attached, and the bridge ports actually appear as devices on those busses, which is really confusing and messes up any kind of mapping to OF nodes in DT.

The second port will probably still not appear, at least not in the latest patches for DT support since it won't be registered unless enabled. Even if enabled it will not be registered unless a link is available, which it isn't in any of the setups that I currently have. I'll need to check with our hardware engineers whether we can hook something up to the second port.

That's probably part of what Stephen mentioned, with this IP being derived from a desktop variant where HyperTransport is actually available.

You've provided a lot of very useful information in this thread and I have a number of things I can try out now to make this work in a more compliant way. Thanks!

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121216/0845e06d/attachment-0001.sig>

No specific reason, it similar to what intel did and clearly shows the port number in a octet of the device id. It just can't be 0.

You can probably bodge around and just register it anyhow, the internal bridge will still show up in linux, even though there is no device behind it.

I'm not sure if PCI-E ports should be ignored if the link is down. The kernel has the ability to re-scan them at runtime, and if the controller has a link up interrupt then it can happen automatically.

I sent through a patch for kirkwood that did this, we hot plug the PCIe device because userspace intializes it.

Jason

So I tried and implemented something along the lines of what you suggested, and guess what? It does work indeed. For some reason even the "imprecise external abort" goes away. Basically what I did is fake the configuration space accesses to 0000:0.0 so that they return semi-valid data. Nothing special yet, just basically returning vendor and product ID, header type, class and other static data.

Initially the implementation was read-only and that still caused the external abort, but adding nop'ed write functions apparently solved that.

What I'll do next is add some caching of written values, so that reading them back actually yields something correct. After that I'll post what I have so that others can look at it or even reuse it.

The problem of matching DT nodes to the root ports still isn't solved, but I'll take another look at that tomorrow.

I don't think the root ports on Tegra support this. Or at least there's no code to support it yet. I'll see if I can at least modify the code to still show the root port on the bus if it is enabled but the link is not available. The DT binding allows each port to be enabled so that if a board configuration doesn't provide a physical connection the software doesn't have to bother initializing it.
available.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121217/207850de/attachment-0001.sig>

Some internal discussion implies this shouldn't be required; here's what they said:

-----Original Message----- (from Chung-Hung Lai)

All PCIE root port registers are in configuration space. To access them, as well as configuration registers of PCIE devices below the bridge, you will need to program the AXI to FPCI BAR mappings defined in AFI. After programmed those, the configuration register space of PCIE fabric are mapped to Tegra's MMIO register space. I take it back that T30 and [elided] have differences - that was for other units, not PCIE.

As Mark said, all AFI's own registers are in MMIO space and can be accessed directly.

Thanks,
Luke

-----Original Message----- (from Mark Van Nostrand)

There are pci bridge registers but also registers to configure the PCIE wrapper. AFAIK all Tegra devices (T20/T30/[elided]) have compliant bridge registers but also need AFI (PCIE wrapper) configuration updates as well.

[trimmed the cc list]

Looking at the driver, it says:

+ * 0x80000000 - 0x80003fff - PCI controller registers
+ * 0x80004000 - 0x80103fff - PCI configuration space
+ * 0x80104000 - 0x80203fff - PCI extended configuration space

And the bridge config regions are at:

+#define RP0_OFFSET 0x0000
+#define RP1_OFFSET 0x1000

And from the first checkin, config space accesses have been split into three designations:
bridge #0 - 0x80000000 + 0
bridge #1 - 0x80000000 + 0x1000
Every other bus - 0x80104000 + [equation]

And based on Thierry's results it looks like once the bridge subordinate bus numbers are setup (via their lower mappings) then the 'every other bus' mapping starts working as expected.

So.. is there a register to be set that lets the bridge configuration be accessed through the [0x80004000 - 0x80103fff] region as bus 0? Presumably a non-PCI device number register?

Is there some provision for providing a host bridge configuration space?

Jason

It looks like some PCI host controller registers do need to be set up (so far the information is related to "type 0" vs. "type 1" configuration accesses, although I'm not familiar with what that means).

Jay will continue to drive this with our HW folks and try and get this working.

So I've been spending some time implementing this along with some of the things that Stephen and I discussed on IRC. However, each time I try to access the type 0 configuration space through the special mapping that Stephen dug up I get the following "imprecise external abort" oops:

[    3.482802] tegra-pcie 80003000.pcie-controller: PCI host bridge to bus 0000:00
[    3.490107] pci_bus 0000:00: root bus resource [io  0x82000000-0x8200ffff]
[    3.517456] pci_bus 0000:00: root bus resource [mem 0xb0000000-0xb7ffffff]
[    3.530644] pci_bus 0000:00: root bus resource [mem 0xb8000000-0xbfffffff pref]
[    3.554191] pci_bus 0000:00: root bus resource [bus 00-ff]
[    3.559693] Unhandled fault: imprecise external abort (0x406) at 0x00034384
[    3.559702] tegra-pcie 80003000.pcie-controller: AXI response decoding error, signature: 00000001
[    3.559708] tegra-pcie 80003000.pcie-controller:   FPCI address: fe00000000
[    3.582435] Internal error: : 406 [#1] PREEMPT SMP ARM
[    3.587560] Modules linked in:
[    3.590616] CPU: 1    Not tainted  (3.7.0-next-20121219-00103-gb307de6-dirty #42)
[    3.591125] usb 1-1.1: new high-speed USB device number 3 using tegra-ehci
[    3.604944] PC is at tegra_pcie_read_conf+0x60/0xd4
[    3.609823] LR is at ioremap_page_range+0xec/0x19c
[    3.614605] pc : [<c0025034>]    lr : [<c01dfcec>]    psr: a0000093
[    3.614605] sp : da645d08  ip : 90000000  fp : c06a8bf4
[    3.626063] r10: 00000000  r9 : da645de0  r8 : dae89400
[    3.631274] r7 : 00000000  r6 : 00000000  r5 : 00000004  r4 : 00000000
[    3.637790] r3 : ffffffff  r2 : 00000000  r1 : e0823000  r0 : e0822000
[    3.644309] Flags: NzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
[    3.651696] Control: 10c5387d  Table: 1a6f404a  DAC: 00000015
[    3.657437] Process kworker/u:3 (pid: 55, stack limit = 0xda644238)

I beefed up the interrupt service routine a bit to output the full FPCI address that the decoding error occurred and it always is the one configured for the type 0 configuration space mapping. I assume that the "Internal error" is due to the "imprecise external abort", but it would be good if somebody could confirm that.

Basically I have modified tegra_pcie_setup_translations() to have this:

	/* Bar 0: type 0 configuration space */
	fpci_bar = 0xfe000000;
	size = resource_size(pcie->cfg[0]);
	axi_address = pcie->cfg[0]->start;
	afi_writel(pcie, axi_address, AFI_AXI_BAR0_START);
	afi_writel(pcie, size >> 12, AFI_AXI_BAR0_SZ);
	afi_writel(pcie, fpci_bar, AFI_FPCI_BAR0);

	/* Bar 1: type 1 configuration space */
	fpci_bar = 0xfe100000;
	size = resource_size(pcie->cfg[1]);
	axi_address = pcie->cfg[1]->start;
	afi_writel(pcie, axi_address, AFI_AXI_BAR1_START);
	afi_writel(pcie, size >> 12, AFI_AXI_BAR1_SZ);
	afi_writel(pcie, fpci_bar, AFI_FPCI_BAR1);

Where pcie->cfg[0] and pcie->cfg[1] are parsed from the following device tree snippet:

	reg = <0x80003000 0x00000800   /* PADS registers */
	       0x80003800 0x00000200   /* AFI registers */
	       0x90000000 0x10000000   /* type 0 configuration space */
	       0xa0000000 0x10000000>; /* type 1 configuration space */

The complete code can be found in my tegra/next branch[0]. Beware that I haven't cleaned up that branch in a while, so anything but looking at the end result is not very recommended =). The branch contains patches (untested) to make Harmony and TrimSlice initialize the PCIe from the device tree, so anyone inclined to give this a spin should be able to do so. If you want to try out the software emulated host bridge, you can just uncomment the "#define EMULATE_HOST_BRIDGE 1" at the top of arch/arm/mach-tegra/pcie.c.

Stephen: Do you think you could dig some more, try to find out what the deal is with these "imprecise external aborts"? Perhaps something else needs to be done in order to activate the type 0 mapping? Also, while looking through some old downstream code from the Vibrante area I saw a HyperTransport document mentioned that contains these mappings. It is referred to as the "AFI IAS" (section 3.2.4 is mentioned specifically). Any chance I could take a look at it?

Thierry

[0]: git://gitorious.org/thierryreding/linux.git
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121220/33bdde75/attachment.sig>

Thierry,

Are you doing below BARs programing for config space or extended config space? pcie->cfg & comments indicates config and fpci_bar = 0xfe0... & size indicates extended config.

Also make sure you add up 0x10000000(according to your below impl.) to pcie->cs/pcie->extcs in pcie_read(write)_conf api so that type1 addresses will actually be mapped to 0xfe1... fpci address.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121221/75c47902/attachment-0001.html>

Jay, can you please wrap your mails at something around 78 characters per line? Anything longer becomes very hard to read and quote.

The programming is for extended configuration space. Using only the extended configuration space mapping works fine even when not accessing the extended registers so we can save one BAR by just not using the regular configuration space mapping.

On a side note, the Tegra TRM says that there are actually 8 BARs, while both upstream and downstream kernel code says that there are only 5. Can you find out which one is true? It would also be nice to get some better documentation for the PCIe controller as the latest versions that are available don't even have the register offsets for the individual registers.

I don't understand. If I map the 40-bit FPCI address 0xfe10000000 to AXI address 0xa0000000 and then map that to a virtual address that I can read from or write to, why would I need to add an offset of 0x10000000 again?

Also Stephen found out that the host bridge does not appear on bus 0, only the root ports do. It would still be nice to access the root ports' configuration space through a proper mapping instead of the extra window but if the host bridge isn't even visible on that bus, then we can just as well stick with the per-root port windows. That way we also save an additional 256 MiB of physical address space.

Thierry

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121221/128ffcdf/attachment.sig>

Dear Thierry Reding,

Thanks for putting your code online. I've started using it already for the Marvell PCIe support, at least the host-bridge.c helpers you've written, and also to have a look at the DT binding you've implemented.

Best regards,

Thomas

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121222/89f751ac/attachment.sig>

That's great. I've been thinking about this some more and one problem we'll be facing is that none of the fake host bridges actually exist and therefore don't have any official PCI IDs. What I did for Tegra was just choose one of the nForce IDs, but that's obviously hackish. One solution would be to convince somebody benevolent to sponsor an ID for this particular purpose. I think Red Hat did so for Virtio.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121228/be93f31d/attachment.sig>

Dear Thierry Reding,

Indeed, that's a remaining problem to solve. Would be great to have some vendor sponsoring a few IDs for that. At least for a fake PCI Host Bridge and a fake PCI-to-PCI bridge.

Thomas

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Is it actually necessary for a host/PCI bridge to show up on the PCI bus as device 0?

Dear Stephen Warren,

I don't know, PCI experts are much better placed than me to answer this question. How is this related to the problem of PCI IDs ?

Best regards,

Thomas

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

If a real PCI device exists, its configuration registers contain the PCI IDs, so you just "use" them. If no real PCI device exists, yet should, so you end up implementing a virtual device, the SW needs to decide which PCI IDs to masquerade as, so you need to come up with some fake IDs.

It isn't actually required to make PCI work, but it enables PCI device matching to DT nodes with the generic code. If we leave out the host bridge, then the matching becomes rather complicated and we'll most likely have to special-case it (in the worst case on a per-mach basis). The PCI core allows this by overriding the default implementations (weak symbols) but doing so will conflict with the multi-platform work. Making it work with multi-platform will probably require the addition of a .of_match() to struct pci_ops or similar.

Faking a host bridge seems like the lesser evil to me.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121229/eba49ec7/attachment.sig>

...

Hmmm. Doesn't the DT node represent the host controller rather than the host bridge, such that any device on a certain PCI bus can easily be mapped back to the relevant host controller device, and hence relevant DT node? Having to "route" this mapping through the host bridge PCI device (which IIRC just acts as a somewhat passive conduit between the host controller and the bus) seems a little odd. But anyway, I guess if that's the way the PCI core is written, and it's non-trivial to change, then I guess yes a virtual device does seem required.

Can you elaborate on this a bit? What do pci bus location bindings look like?

BTW, does anyone have working links for the various PDFs (ePAR, PCI bindings, etc) for open firmware stuff, they all seem broken these days..

Cheers,
Jason

Basically what you do is describe the PCI bus hierarchy using DT nodes, where the reg properties are used to match device nodes to their PCI devices. You can see a basic example for the controller here[0] and an actual board DT here[1].

Contrary to what I said earlier, more recent tests seem to indicate that the matching does indeed work even without the fake host bridge device. Perhaps one of the changes or cleanups caused this. I'll do some further testing to confirm this.

The ePAPR 1.1 and the PCI bindings are available here[2] and here[3].

[0]: https://gitorious.org/thierryreding/linux/blobs/tegra/next/arch/arm/boot/dts/tegra20.dtsi#line369
[1]: https://gitorious.org/thierryreding/linux/blobs/tegra/next/arch/arm/boot/dts/tegra20-tec.dts#line67
[2]: https://www.power.org/wp-content/uploads/2012/07/Power_ePAPR_APPROVED_v1.1.pdf
[3]: http://www.openfirmware.org/ofwg/bindings/pci/pci2_1.pdf

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130103/612829a7/attachment.sig>

On 12/20/2012 08:32 AM, Thierry Reding wrote:
...

I've confirmed with the HW people that on Tegra, there is no host/PCIe bridge per se (or at least, no device that exposes PCIe configuration-style registers, either through type 0 accesses, or a special-case dedicated register set), and that the PCIe root ports don't respond to type 0 configuration transactions; their PCIe config registers must be accessed through their special-case dedicated register set.

But none of the above matches the (sparse) documentation. The TRM clearly states that each root port has a 4 KiB window that should be used to access the root port's configuration space. The driver supports that information.

I can also easily refute it by modifying the configuration space accessor to look like this:

	static int tegra_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
					int where, int size, u32 *value)
	{
		struct tegra_pcie *pcie = sys_to_pcie(bus->sysdata);
		void __iomem *addr = NULL;

		if (where >= 0x100)
			return PCIBIOS_BAD_REGISTER_NUMBER;

		addr = pcie->cs + PCIE_CONF_BUS(bus->number) +
				  PCIE_CONF_DEV(PCI_SLOT(devfn)) +
				  PCIE_CONF_FUNC(PCI_FUNC(devfn)) +
				  PCIE_CONF_REG(where);

		*value = readl(addr);

		if (size == 1)
			*value = (*value >> (8 * (where & 3))) & 0xff;
		else if (size == 2)
			*value = (*value >> (8 * (where & 3))) & 0xffff;

		return PCIBIOS_SUCCESSFUL;
	}

and booting with that modification, which yields:

[    3.097784] pci 0000:00:00.0: [1556:4711] type 01 class 0x060400

Which is the PLDA bridge within the FPGA, *not* the Tegra host bridge.

So either we're doing something really wrong when setting up the FPCI BAR mappings or you guys need to get your information right.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121218/b78f1872/attachment.sig>

Hi everyone,

Thomas' RFC series incited me to put in some more work on the Tegra PCIe controller driver. This in turn brought up an old issue: should we move PCIe controller drivers to some central location rather than keeping them in machine-specific directories? Last time I brought this up there was no consensus but I really think there's some value in keeping these drivers in a common location, especially now that a few people are starting to work on similar problems. It may also provide some more momentum to get the various DT implementations factored out of various architectures.

Stephen proposed drivers/pci/host on IRC and I second that.

Bjorn, is this something you would be on board with?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130103/5b28ff44/attachment.sig>

Sounds fine to me.

Great! Thomas, does that work for you as well?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130103/b057b741/attachment-0001.sig>

Dear Thierry Reding,

I for sure would be interested in a direction such as this one being taken for PCI support. Having drivers in a common directory will help comparing them, extracting common patterns, etc.

Best regards,

Thomas

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Agreed, I like that, too. There is a little problem with cross-platform PCI hosts still because some portion of the interface is still arch specific, but I don't see this as a show-stopper for moving drivers there. The ARM specific drivers can still use the ARM interfaces for now, until everything is made generic. If they are going to be shared with other architectures, those can use the same interfaces that ARM has. I think Microblaze, ARC and Metag are strong candidates for doing this.

Arnd

Great, sounds like a plan. I'll move Tegra PCIe into drivers/pci/host then.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130103/bf0e6ef1/attachment.sig>