[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thomas Petazzoni]

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

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Thomas Petazzoni]

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.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: linux-kernel at vger.kernel.org
---
 lib/devres.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/devres.c b/lib/devres.c
index 80b9c76..5639c3e 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -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

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Arnd Bergmann]

On Friday 07 December 2012, Thomas Petazzoni wrote:
> 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.

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

> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Cc: linux-kernel at vger.kernel.org
> ---
>  lib/devres.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/devres.c b/lib/devres.c
> index 80b9c76..5639c3e 100644
> --- a/lib/devres.c
> +++ b/lib/devres.c
> @@ -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 */

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Tue, 11 Dec 2012 10:43:49 +0000, Arnd Bergmann wrote:
> On Friday 07 December 2012, Thomas Petazzoni wrote:
> > 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.
> 
> 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.

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

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Arnd Bergmann]

On Tuesday 11 December 2012, Thomas Petazzoni wrote:
> On Tue, 11 Dec 2012 10:43:49 +0000, Arnd Bergmann wrote:
> > On Friday 07 December 2012, Thomas Petazzoni wrote:
> > > 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.
> > 
> > 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.
> 
> 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?

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.

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

Sorry, I meant pcim_iomap.

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

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

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Russell King - ARM Linux]

On Tue, Dec 11, 2012 at 04:15:02PM +0000, Arnd Bergmann wrote:
> On Tuesday 11 December 2012, Thomas Petazzoni wrote:
> > On Tue, 11 Dec 2012 10:43:49 +0000, Arnd Bergmann wrote:
> > > On Friday 07 December 2012, Thomas Petazzoni wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > 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?
> 
> 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.

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.

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Thomas Petazzoni]

Russell,

On Tue, 11 Dec 2012 16:23:25 +0000, Russell King - ARM Linux wrote:

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

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

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Russell King - ARM Linux]

On Tue, Dec 11, 2012 at 05:38:19PM +0100, Thomas Petazzoni wrote:
> Russell,
> 
> On Tue, 11 Dec 2012 16:23:25 +0000, Russell King - ARM Linux wrote:
> 
> > > * 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.
> > 
> > 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.
> 
> 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?

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

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Alan Cox]

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

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.

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

There is if you ask the PCI bridge or beyond.

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

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

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Arnd Bergmann]

On Tuesday 11 December 2012, Alan Cox wrote:
> 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.

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

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Arnd Bergmann]

On Tuesday 11 December 2012, Russell King - ARM Linux wrote:
> 
> On Tue, Dec 11, 2012 at 04:15:02PM +0000, Arnd Bergmann wrote:
> > On Tuesday 11 December 2012, Thomas Petazzoni wrote:
> > > On Tue, 11 Dec 2012 10:43:49 +0000, Arnd Bergmann wrote:
> > > > On Friday 07 December 2012, Thomas Petazzoni wrote:
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > 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?
> > 
> > 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.
> 
> 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.

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

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

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

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Tue, 11 Dec 2012 16:15:02 +0000, Arnd Bergmann wrote:

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

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

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

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.

> > 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.
> 
> Sorry, I meant pcim_iomap.
> 
> > 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.
> 
> 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.

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

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Russell King - ARM Linux]

On Tue, Dec 11, 2012 at 05:30:13PM +0100, Thomas Petazzoni wrote:
> 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

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

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

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.

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Alan Cox]

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

And on a lot of devices the LPC bus.

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

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

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Arnd Bergmann]

On Tuesday 11 December 2012, Alan Cox wrote:
> > 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.
> 
> 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).

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

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Russell King - ARM Linux]

On Tue, Dec 11, 2012 at 05:30:13PM +0100, Thomas Petazzoni wrote:
> 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

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

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Russell King - ARM Linux]

On Tue, Dec 11, 2012 at 10:43:49AM +0000, Arnd Bergmann wrote:
> On Friday 07 December 2012, Thomas Petazzoni wrote:
> > 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.
> 
> 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.

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.

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Alan Cox]

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

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Russell King - ARM Linux]

On Tue, Dec 11, 2012 at 05:16:10PM +0000, Alan Cox wrote:
> > 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.

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

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Alan Cox]

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

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

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

Don't blame you 8)

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Russell King - ARM Linux]

On Tue, Dec 11, 2012 at 05:45:09PM +0000, Alan Cox wrote:
> > 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.
> 
> It's a cookie - this isn't a problem, you can support it via the mapping
> approah. Whether you want to is another question.

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

[RFC v1 02/16] clk: mvebu: create parent-child relation for PCIe clocks on Armada 370

[Thomas Petazzoni]

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.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Mike Turquette <mturquette@linaro.org>
---
 drivers/clk/mvebu/clk-gating-ctrl.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/mvebu/clk-gating-ctrl.c b/drivers/clk/mvebu/clk-gating-ctrl.c
index c6d3c26..a328be5 100644
--- a/drivers/clk/mvebu/clk-gating-ctrl.c
+++ b/drivers/clk/mvebu/clk-gating-ctrl.c
@@ -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

[RFC v1 03/16] arm: plat-orion: introduce WIN_CTRL_ENABLE in address mapping code

[Thomas Petazzoni]

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

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/plat-orion/addr-map.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/plat-orion/addr-map.c b/arch/arm/plat-orion/addr-map.c
index febe386..4dec3db 100644
--- a/arch/arm/plat-orion/addr-map.c
+++ b/arch/arm/plat-orion/addr-map.c
@@ -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

[RFC v1 04/16] arm: plat-orion: refactor the orion_disable_wins() function

[Thomas Petazzoni]

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.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/plat-orion/addr-map.c |   35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/arch/arm/plat-orion/addr-map.c b/arch/arm/plat-orion/addr-map.c
index 4dec3db..dd98638 100644
--- a/arch/arm/plat-orion/addr-map.c
+++ b/arch/arm/plat-orion/addr-map.c
@@ -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

[RFC v1 05/16] arm: plat-orion: introduce orion_{alloc, free}_cpu_win() functions

[Thomas Petazzoni]

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.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/plat-orion/addr-map.c              |   50 +++++++++++++++++++++++++++
 arch/arm/plat-orion/include/plat/addr-map.h |    7 ++++
 2 files changed, 57 insertions(+)

diff --git a/arch/arm/plat-orion/addr-map.c b/arch/arm/plat-orion/addr-map.c
index dd98638..8f4ef82 100644
--- a/arch/arm/plat-orion/addr-map.c
+++ b/arch/arm/plat-orion/addr-map.c
@@ -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,
diff --git a/arch/arm/plat-orion/include/plat/addr-map.h b/arch/arm/plat-orion/include/plat/addr-map.h
index b76c065..f8bb539 100644
--- a/arch/arm/plat-orion/include/plat/addr-map.h
+++ b/arch/arm/plat-orion/include/plat/addr-map.h
@@ -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

[RFC v1 05/16] arm: plat-orion: introduce orion_{alloc,free}_cpu_win() functions

[Andrew Lunn]

Hi Thomas

> + * 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;
> +}


Not a big issue, but i would of put the test for WIN_CTRL_ENABLE
inside the matched winbase if statement, also differing the readl for
ctrl to only when its needed.

     Andrew

[RFC v1 05/16] arm: plat-orion: introduce orion_{alloc,free}_cpu_win() functions

[Thomas Petazzoni]

Dear Andrew Lunn,

On Sat, 8 Dec 2012 12:53:06 +0100, Andrew Lunn wrote:

> > + * 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;
> > +}
> 
> Not a big issue, but i would of put the test for WIN_CTRL_ENABLE
> inside the matched winbase if statement, also differing the readl for
> ctrl to only when its needed.

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

[RFC v1 06/16] arm: mvebu: add functions to alloc/free PCIe decoding windows

[Thomas Petazzoni]

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.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/mach-mvebu/addr-map.c |  156 ++++++++++++++++++++++++++++++++++++++--
 arch/arm/mach-mvebu/common.h   |    4 ++
 2 files changed, 156 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-mvebu/addr-map.c b/arch/arm/mach-mvebu/addr-map.c
index ab9b3bd..c21aa7b 100644
--- a/arch/arm/mach-mvebu/addr-map.c
+++ b/arch/arm/mach-mvebu/addr-map.c
@@ -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;
diff --git a/arch/arm/mach-mvebu/common.h b/arch/arm/mach-mvebu/common.h
index aa27bc2..3cef81a 100644
--- a/arch/arm/mach-mvebu/common.h
+++ b/arch/arm/mach-mvebu/common.h
@@ -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

[RFC v1 07/16] arm: plat-orion: make common PCIe code usable on mvebu

[Thomas Petazzoni]

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.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/plat-orion/Makefile |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/plat-orion/Makefile b/arch/arm/plat-orion/Makefile
index a82cecb..1aca22b 100644
--- a/arch/arm/plat-orion/Makefile
+++ b/arch/arm/plat-orion/Makefile
@@ -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

[RFC v1 08/16] arm: mvebu: the core PCIe driver

[Thomas Petazzoni]

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.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 .../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

diff --git a/Documentation/devicetree/bindings/pci/armada-370-xp-pcie.txt b/Documentation/devicetree/bindings/pci/armada-370-xp-pcie.txt
new file mode 100644
index 0000000..daf0775
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/armada-370-xp-pcie.txt
@@ -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";
+	};
+};
+
diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
index 5dcb369..e458ede 100644
--- a/arch/arm/mach-mvebu/Makefile
+++ b/arch/arm/mach-mvebu/Makefile
@@ -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
diff --git a/arch/arm/mach-mvebu/pcie.c b/arch/arm/mach-mvebu/pcie.c
new file mode 100644
index 0000000..d88f800
--- /dev/null
+++ b/arch/arm/mach-mvebu/pcie.c
@@ -0,0 +1,306 @@
+/*
+ * PCIe driver for Marvell Armada 370 and Armada XP SoCs
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/of_address.h>
+#include <linux/of_pci.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <plat/pcie.h>
+#include "common.h"
+
+struct pcie_port {
+	u8		  root_bus_nr;
+	void __iomem	 *base;
+	spinlock_t	  conf_lock;
+	int		  irq;
+	struct resource	  res;
+	int               haslink;
+	u32               port;
+	u32               lane;
+	struct clk       *clk;
+};
+
+static struct pcie_port *pcie_ports;
+
+static int pcie_valid_config(struct pcie_port *pp, int bus, int dev)
+{
+	/*
+	 * Don't go out when trying to access --
+	 * 1. nonexisting device on local bus
+	 * 2. where there's no device connected (no link)
+	 */
+	if (bus == pp->root_bus_nr && dev == 0)
+		return 1;
+
+	if (!orion_pcie_link_up(pp->base))
+		return 0;
+
+	if (bus == pp->root_bus_nr && dev != 1)
+		return 0;
+
+	return 1;
+}
+
+/*
+ * PCIe config cycles are done by programming the PCIE_CONF_ADDR register
+ * and then reading the PCIE_CONF_DATA register. Need to make sure these
+ * transactions are atomic.
+ */
+static int pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
+			int size, u32 *val)
+{
+	struct pci_sys_data *sys = bus->sysdata;
+	struct pcie_port *pp = sys->private_data;
+	unsigned long flags;
+	int ret;
+
+	if (pcie_valid_config(pp, bus->number, PCI_SLOT(devfn)) == 0) {
+		*val = 0xffffffff;
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	}
+
+	spin_lock_irqsave(&pp->conf_lock, flags);
+	ret = orion_pcie_rd_conf(pp->base, bus, devfn, where, size, val);
+	spin_unlock_irqrestore(&pp->conf_lock, flags);
+
+	return ret;
+}
+
+static int pcie_wr_conf(struct pci_bus *bus, u32 devfn,
+			int where, int size, u32 val)
+{
+	struct pci_sys_data *sys = bus->sysdata;
+	struct pcie_port *pp = sys->private_data;
+	unsigned long flags;
+	int ret;
+
+	if (pcie_valid_config(pp, bus->number, PCI_SLOT(devfn)) == 0)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	spin_lock_irqsave(&pp->conf_lock, flags);
+	ret = orion_pcie_wr_conf(pp->base, bus, devfn, where, size, val);
+	spin_unlock_irqrestore(&pp->conf_lock, flags);
+
+	return ret;
+}
+
+static struct pci_ops pcie_ops = {
+	.read = pcie_rd_conf,
+	.write = pcie_wr_conf,
+};
+
+/*
+ * Returns 0 when the device could not be initialized, 1 when
+ * initialization is successful
+ */
+static int __init armada_370_xp_pcie_setup(int nr, struct pci_sys_data *sys)
+{
+	struct pcie_port *port = &pcie_ports[nr];
+	unsigned long membase, iobase;
+	int ret;
+
+	if (!port->haslink)
+		return 0;
+
+	sys->private_data = port;
+	port->root_bus_nr = sys->busnr;
+	spin_lock_init(&port->conf_lock);
+
+	ret = armada_370_xp_alloc_pcie_window(port->port, port->lane,
+					      IORESOURCE_MEM, SZ_64M, &membase);
+	if (ret) {
+		pr_err("PCIe%d.%d: Cannot get memory window, device disabled\n",
+		       port->port, port->lane);
+		return 0;
+	}
+
+	ret = armada_370_xp_alloc_pcie_window(port->port, port->lane,
+					      IORESOURCE_IO, SZ_64K, &iobase);
+	if (ret) {
+		pr_err("PCIe%d.%d: Cannot get I/O window, device disabled\n",
+		       port->port, port->lane);
+		armada_370_xp_free_pcie_window(IORESOURCE_MEM, membase, SZ_64M);
+		return 0;
+	}
+
+	port->res.name = kasprintf(GFP_KERNEL, "PCIe %d.%d MEM",
+				   port->port, port->lane);
+	if (!port->res.name) {
+		armada_370_xp_free_pcie_window(IORESOURCE_IO, iobase, SZ_64K);
+		armada_370_xp_free_pcie_window(IORESOURCE_MEM, membase, SZ_64M);
+		return 0;
+	}
+
+	port->res.start = membase;
+	port->res.end = membase + SZ_32M - 1;
+	port->res.flags = IORESOURCE_MEM;
+
+	pci_ioremap_io(SZ_64K * sys->busnr, iobase);
+
+	if (request_resource(&iomem_resource, &port->res)) {
+		pr_err("PCIe%d.%d: Cannot request memory resource\n",
+		       port->port, port->lane);
+		kfree(port->res.name);
+		armada_370_xp_free_pcie_window(IORESOURCE_IO, iobase, SZ_64K);
+		armada_370_xp_free_pcie_window(IORESOURCE_MEM, membase, SZ_64M);
+		return 0;
+	}
+
+	pci_add_resource_offset(&sys->resources, &port->res, sys->mem_offset);
+
+	orion_pcie_set_local_bus_nr(port->base, sys->busnr);
+	orion_pcie_setup(port->base);
+
+	return 1;
+}
+
+static void __devinit rc_pci_fixup(struct pci_dev *dev)
+{
+	/*
+	 * Prevent enumeration of root complex.
+	 */
+	if (dev->bus->parent == NULL && dev->devfn == 0) {
+		int i;
+
+		for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+			dev->resource[i].start = 0;
+			dev->resource[i].end   = 0;
+			dev->resource[i].flags = 0;
+		}
+	}
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MARVELL, PCI_ANY_ID, rc_pci_fixup);
+
+static int __init armada_370_xp_pcie_map_irq(const struct pci_dev *dev, u8 slot,
+					     u8 pin)
+{
+	struct pci_sys_data *sys = dev->sysdata;
+	struct pcie_port *port = sys->private_data;
+
+	return port->irq;
+}
+
+static struct hw_pci armada_370_xp_pci __initdata = {
+	.setup		= armada_370_xp_pcie_setup,
+	.map_irq	= armada_370_xp_pcie_map_irq,
+	.ops            = &pcie_ops,
+};
+
+static int __init armada_370_xp_pcie_probe(struct platform_device *pdev)
+{
+	struct device_node *child;
+	int nports, i;
+
+	nports = 0;
+	for_each_child_of_node(pdev->dev.of_node, child) {
+		if (!of_device_is_available(child))
+			continue;
+		nports++;
+	}
+
+	pcie_ports = devm_kzalloc(&pdev->dev, nports * sizeof(*pcie_ports),
+				  GFP_KERNEL);
+	if (!pcie_ports)
+		return -ENOMEM;
+
+	i = 0;
+	for_each_child_of_node(pdev->dev.of_node, child) {
+		struct pcie_port *port = &pcie_ports[i];
+
+		if (!of_device_is_available(child))
+			continue;
+
+		if (of_property_read_u32(child, "marvell,pcie-port",
+					 &port->port))
+			continue;
+
+		if (of_property_read_u32(child, "marvell,pcie-lane",
+					 &port->lane))
+			port->lane = 0;
+
+		port->base = of_iomap(child, 0);
+		if (!port->base) {
+			dev_err(&pdev->dev, "PCIe%d.%d: cannot map registers\n",
+				port->port, port->lane);
+			continue;
+		}
+
+		if (orion_pcie_link_up(port->base)) {
+			port->haslink = 1;
+			dev_info(&pdev->dev, "PCIe%d.%d: link up\n",
+				 port->port, port->lane);
+		} else {
+			port->haslink = 0;
+			dev_info(&pdev->dev, "PCIe%d.%d: link down\n",
+				 port->port, port->lane);
+			iounmap(port->base);
+			continue;
+		}
+
+		port->irq = irq_of_parse_and_map(child, 0);
+		if (port->irq == 0) {
+			dev_err(&pdev->dev, "PCIe%d.%d: cannot parse and map IRQ\n",
+				port->port, port->lane);
+			iounmap(port->base);
+			port->haslink = 0;
+			continue;
+		}
+
+		port->clk = of_clk_get_by_name(child, NULL);
+		if (!port->clk) {
+			dev_err(&pdev->dev, "PCIe%d.%d: cannot get clock\n",
+			       port->port, port->lane);
+			irq_dispose_mapping(port->irq);
+			iounmap(port->base);
+			port->haslink = 0;
+			continue;
+		}
+
+		clk_prepare_enable(port->clk);
+
+		i++;
+	}
+
+	armada_370_xp_pci.nr_controllers = nports;
+	pci_common_init(&armada_370_xp_pci);
+
+	return 0;
+}
+
+static const struct of_device_id armada_370_xp_pcie_of_match_table[] = {
+	{ .compatible = "marvell,armada-370-xp-pcie", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, armada_370_xp_pcie_of_match_table);
+
+static struct platform_driver armada_370_xp_pcie_driver = {
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = "armada-370-xp-pcie",
+		.of_match_table =
+		   of_match_ptr(armada_370_xp_pcie_of_match_table),
+	},
+};
+
+static int armada_370_xp_pcie_init(void)
+{
+	return platform_driver_probe(&armada_370_xp_pcie_driver,
+				     armada_370_xp_pcie_probe);
+}
+
+subsys_initcall(armada_370_xp_pcie_init);
+
+MODULE_AUTHOR("Thomas Petazzoni <thomas.petazzoni@free-electrons.com>");
+MODULE_DESCRIPTION("Marvell Armada 370/XP PCIe driver");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5

[RFC v1 08/16] arm: mvebu: the core PCIe driver

[Andrew Lunn]

> +static int pcie_valid_config(struct pcie_port *pp, int bus, int dev)
> +{
> +	/*
> +	 * Don't go out when trying to access --
> +	 * 1. nonexisting device on local bus
> +	 * 2. where there's no device connected (no link)
> +	 */
> +	if (bus == pp->root_bus_nr && dev == 0)
> +		return 1;
> +
> +	if (!orion_pcie_link_up(pp->base))
> +		return 0;
> +
> +	if (bus == pp->root_bus_nr && dev != 1)
> +		return 0;
> +
> +	return 1;
> +}
> +
> +/*
> + * PCIe config cycles are done by programming the PCIE_CONF_ADDR register
> + * and then reading the PCIE_CONF_DATA register. Need to make sure these
> + * transactions are atomic.
> + */
> +static int pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
> +			int size, u32 *val)
> +{
> +	struct pci_sys_data *sys = bus->sysdata;
> +	struct pcie_port *pp = sys->private_data;
> +	unsigned long flags;
> +	int ret;
> +
> +	if (pcie_valid_config(pp, bus->number, PCI_SLOT(devfn)) == 0) {
> +		*val = 0xffffffff;
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +	}
> +
> +	spin_lock_irqsave(&pp->conf_lock, flags);
> +	ret = orion_pcie_rd_conf(pp->base, bus, devfn, where, size, val);
> +	spin_unlock_irqrestore(&pp->conf_lock, flags);
> +
> +	return ret;
> +}
> +
> +static int pcie_wr_conf(struct pci_bus *bus, u32 devfn,
> +			int where, int size, u32 val)
> +{
> +	struct pci_sys_data *sys = bus->sysdata;
> +	struct pcie_port *pp = sys->private_data;
> +	unsigned long flags;
> +	int ret;
> +
> +	if (pcie_valid_config(pp, bus->number, PCI_SLOT(devfn)) == 0)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	spin_lock_irqsave(&pp->conf_lock, flags);
> +	ret = orion_pcie_wr_conf(pp->base, bus, devfn, where, size, val);
> +	spin_unlock_irqrestore(&pp->conf_lock, flags);
> +
> +	return ret;
> +}
> +
> +static struct pci_ops pcie_ops = {
> +	.read = pcie_rd_conf,
> +	.write = pcie_wr_conf,
> +};

Hi Thomas

pcie_rd_conf, pcie_wr_conf and pci_ops_pcie_ops are common on orion5x,
dove and kirkwood. Could you maybe factor these functions out and
place them in plat-orion/pcie.c?

> +
> +static void __devinit rc_pci_fixup(struct pci_dev *dev)
> +{
> +	/*
> +	 * Prevent enumeration of root complex.
> +	 */
> +	if (dev->bus->parent == NULL && dev->devfn == 0) {
> +		int i;
> +
> +		for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> +			dev->resource[i].start = 0;
> +			dev->resource[i].end   = 0;
> +			dev->resource[i].flags = 0;
> +		}
> +	}
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MARVELL, PCI_ANY_ID, rc_pci_fixup);

rc_pci_fixup is also identical for all orion platforms, so is another
candidate for plat-orion/pcie.c

These are all exact copies, so are easy to cleanup. With more effort
there is more code which can be refactored and centralized, but that
is maybe too much work for the moment?

Thanks

   Andrew

[RFC v1 08/16] arm: mvebu: the core PCIe driver

[Thomas Petazzoni]

Dear Andrew Lunn,

On Mon, 10 Dec 2012 09:28:47 +0100, Andrew Lunn wrote:

> pcie_rd_conf, pcie_wr_conf and pci_ops_pcie_ops are common on orion5x,
> dove and kirkwood. Could you maybe factor these functions out and
> place them in plat-orion/pcie.c?

Indeed.

> rc_pci_fixup is also identical for all orion platforms, so is another
> candidate for plat-orion/pcie.c

Sure.

> These are all exact copies, so are easy to cleanup. With more effort
> there is more code which can be refactored and centralized, but that
> is maybe too much work for the moment?

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

[RFC v1 08/16] arm: mvebu: the core PCIe driver

[Jason Gunthorpe]

On Fri, Dec 07, 2012 at 11:04:31PM +0100, Thomas Petazzoni wrote:

> +static void __devinit rc_pci_fixup(struct pci_dev *dev)
> +{
> +	/*
> +	 * Prevent enumeration of root complex.
> +	 */
> +	if (dev->bus->parent == NULL && dev->devfn == 0) {
> +		int i;
> +
> +		for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> +			dev->resource[i].start = 0;
> +			dev->resource[i].end   = 0;
> +			dev->resource[i].flags = 0;
> +		}
> +	}
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MARVELL, PCI_ANY_ID, rc_pci_fixup);

You should grab my fixup for this function in the kirkwood code:

1dc831bf53fddcc6443f74a39e72db5bcea4f15d

Regards,
Jason

[RFC v1 08/16] arm: mvebu: the core PCIe driver

[Arnd Bergmann]

On Friday 07 December 2012, Thomas Petazzoni wrote:
> 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.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@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.

> diff --git a/Documentation/devicetree/bindings/pci/armada-370-xp-pcie.txt b/Documentation/devicetree/bindings/pci/armada-370-xp-pcie.txt
> new file mode 100644
> index 0000000..daf0775
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/armada-370-xp-pcie.txt
> @@ -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.

A PCI host controller should set #interrupt-cells=<1>, #size-cells=<2>
and #address-cells=<3>, and provide an interrupt map that describes how
the intA-intD lines for each slot are connected to the host interrupts.

	Arnd

[RFC v1 08/16] arm: mvebu: the core PCIe driver

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Tue, 11 Dec 2012 10:56:58 +0000, Arnd Bergmann wrote:

> > 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.
> > 
> > Signed-off-by: Thomas Petazzoni
> > <thomas.petazzoni@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.

[...]

> A PCI host controller should set #interrupt-cells=<1>, #size-cells=<2>
> and #address-cells=<3>, and provide an interrupt map that describes
> how the intA-intD lines for each slot are connected to the host
> interrupts.

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

[RFC v1 08/16] arm: mvebu: the core PCIe driver

[Jason Gunthorpe]

On Wed, Dec 12, 2012 at 04:58:33PM +0100, Thomas Petazzoni wrote:

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

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.

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

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

[RFC v1 08/16] arm: mvebu: the core PCIe driver

[Arnd Bergmann]

On Wednesday 12 December 2012, Jason Gunthorpe wrote:

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

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

> > 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.
> 
> 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 */>;

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

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.

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

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

	Arnd

[RFC v1 08/16] arm: mvebu: the core PCIe driver

[Jason Gunthorpe]

On Thu, Dec 13, 2012 at 02:58:54PM +0000, Arnd Bergmann wrote:

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

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

[RFC v1 08/16] arm: mvebu: the core PCIe driver

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Thu, 13 Dec 2012 10:40:32 -0700, Jason Gunthorpe wrote:

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

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

[RFC v1 08/16] arm: mvebu: the core PCIe driver

[Rob Herring]

On 12/12/2012 03:51 PM, Jason Gunthorpe wrote:
> On Wed, Dec 12, 2012 at 04:58:33PM +0100, Thomas Petazzoni wrote:
> 
>> 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.
> 
> 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.

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

[RFC v1 08/16] arm: mvebu: the core PCIe driver

[Arnd Bergmann]

On Wednesday 12 December 2012, Thomas Petazzoni wrote:
> Dear Arnd Bergmann,
> 
> On Tue, 11 Dec 2012 10:56:58 +0000, Arnd Bergmann wrote:
> 
> > > 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.
> > > 
> > > Signed-off-by: Thomas Petazzoni
> > > <thomas.petazzoni@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.
> 
> [...]
> 
> > A PCI host controller should set #interrupt-cells=<1>, #size-cells=<2>
> > and #address-cells=<3>, and provide an interrupt map that describes
> > how the intA-intD lines for each slot are connected to the host
> > interrupts.
> 
> 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.

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?

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

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.

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

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.

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

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

[RFC v1 08/16] arm: mvebu: the core PCIe driver

[Jason Gunthorpe]

On Thu, Dec 13, 2012 at 12:19:39PM +0000, Arnd Bergmann wrote:
 
> Do those ten ports have a shared I/O space window, or does each one
> have its own 64K? 

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

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

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

[RFC v1 08/16] arm: mvebu: the core PCIe driver

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Thu, 13 Dec 2012 10:54:42 -0700, Jason Gunthorpe wrote:
> On Thu, Dec 13, 2012 at 12:19:39PM +0000, Arnd Bergmann wrote:
>  
> > Do those ten ports have a shared I/O space window, or does each one
> > have its own 64K? 
> 
> The smallest mbus routing window size is 64k, and you need to use a
> mbus routing window per PEX to decode into IO.

Correct.

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

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

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

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

[RFC v1 08/16] arm: mvebu: the core PCIe driver

[Arnd Bergmann]

On Thursday 13 December 2012, Thomas Petazzoni wrote:
> Dear Jason Gunthorpe,
> 
> On Thu, 13 Dec 2012 10:54:42 -0700, Jason Gunthorpe wrote:
> > On Thu, Dec 13, 2012 at 12:19:39PM +0000, Arnd Bergmann wrote:
> >  
> > > Do those ten ports have a shared I/O space window, or does each one
> > > have its own 64K? 
> > 
> > The smallest mbus routing window size is 64k, and you need to use a
> > mbus routing window per PEX to decode into IO.
> 
> Correct.
> 
> > 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.... :|
> 
> Hum, not sure to follow you here. What sort of finer granularity does
> PCIe requires?

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.

> > 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. :(
> 
> Does that mean that your earlier suggestion of emulating a PCI-to-PCI
> bridge in software is no longer your preferred suggestion?

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

[RFC v1 08/16] arm: mvebu: the core PCIe driver

[Jason Gunthorpe]

On Thu, Dec 13, 2012 at 09:46:05PM +0000, Arnd Bergmann wrote:

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

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

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

The kirkwood docs say windows must never overlap.

> > > 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. :(
> > 
> > Does that mean that your earlier suggestion of emulating a PCI-to-PCI
> > bridge in software is no longer your preferred suggestion?
> 
> 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.

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

[RFC v1 09/16] arm: mvebu: PCIe support is now available on mvebu

[Thomas Petazzoni]

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.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/mach-mvebu/Kconfig |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
index 440b13e..f12e475 100644
--- a/arch/arm/mach-mvebu/Kconfig
+++ b/arch/arm/mach-mvebu/Kconfig
@@ -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

[RFC v1 10/16] arm: mvebu: add PCIe Device Tree informations for Armada 370

[Thomas Petazzoni]

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

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/boot/dts/armada-370.dtsi |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index 636cf7d..2e3c8e0 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -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

[RFC v1 11/16] arm: mvebu: add PCIe Device Tree informations for Armada XP

[Thomas Petazzoni]

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.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 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(+)

diff --git a/arch/arm/boot/dts/armada-xp-mv78230.dtsi b/arch/arm/boot/dts/armada-xp-mv78230.dtsi
index c45c7b4..a4c4fca 100644
--- a/arch/arm/boot/dts/armada-xp-mv78230.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78230.dtsi
@@ -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";
+			};
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/armada-xp-mv78260.dtsi b/arch/arm/boot/dts/armada-xp-mv78260.dtsi
index a2aee57..d799a29 100644
--- a/arch/arm/boot/dts/armada-xp-mv78260.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78260.dtsi
@@ -85,5 +85,77 @@
 			#interrupts-cells = <2>;
 			interrupts = <24>;
 		};
+
+		/*
+		 * MV78260 has 3 PCIe units Gen2.0: Two units can be
+		 * configured as x4 or quad x1 lanes. One unit is
+		 * x4/x1.
+		 */
+		pcie-controller {
+			compatible = "marvell,armada-370-xp-pcie";
+			status = "okay";
+			#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 */
+				  0x12000 0xd0082000 0x2000 /* port3x1_port0 */>;
+
+			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";
+			};
+
+			pcie3 at 0xd0082000 {
+				reg = <0x12000 0x2000>;
+				interrupts = <103>;
+				clocks = <&gateclk 8>;
+				marvell,pcie-port = <3>;
+				marvell,pcie-lane = <0>;
+				status = "disabled";
+			};
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/armada-xp-mv78460.dtsi b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
index da03a12..c2579d7 100644
--- a/arch/arm/boot/dts/armada-xp-mv78460.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
@@ -100,5 +100,117 @@
 			#interrupts-cells = <2>;
 			interrupts = <24>;
 		};
+
+		/*
+		 * MV78460 has 4 PCIe units Gen2.0: Two units can be
+		 * configured as x4 or quad x1 lanes. Two units are
+		 * 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 */
+				  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";
+			};
+		};
 	};
  };
-- 
1.7.9.5

[RFC v1 12/16] arm: mvebu: PCIe Device Tree informations for OpenBlocks AX3-4

[Thomas Petazzoni]

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.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
index b42652f..d9912df 100644
--- a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
+++ b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
@@ -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

[RFC v1 13/16] arm: mvebu: PCIe Device Tree informations for Armada XP DB

[Thomas Petazzoni]

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.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/boot/dts/armada-xp-db.dts |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/arch/arm/boot/dts/armada-xp-db.dts b/arch/arm/boot/dts/armada-xp-db.dts
index 8e53b25..cd378ae 100644
--- a/arch/arm/boot/dts/armada-xp-db.dts
+++ b/arch/arm/boot/dts/armada-xp-db.dts
@@ -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

[RFC v1 14/16] arm: mvebu: PCIe Device Tree informations for Armada 370 Mirabox

[Thomas Petazzoni]

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.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-mirabox.dts |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370-mirabox.dts b/arch/arm/boot/dts/armada-370-mirabox.dts
index 3b40713..6de4b74 100644
--- a/arch/arm/boot/dts/armada-370-mirabox.dts
+++ b/arch/arm/boot/dts/armada-370-mirabox.dts
@@ -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

[RFC v1 15/16] arm: mvebu: PCIe Device Tree informations for Armada 370 DB

[Thomas Petazzoni]

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.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-db.dts |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370-db.dts b/arch/arm/boot/dts/armada-370-db.dts
index 0004402..56f2c14 100644
--- a/arch/arm/boot/dts/armada-370-db.dts
+++ b/arch/arm/boot/dts/armada-370-db.dts
@@ -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

[RFC v1 16/16] arm: mvebu: update defconfig with PCI and USB support

[Thomas Petazzoni]

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.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/configs/mvebu_defconfig |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/configs/mvebu_defconfig b/arch/arm/configs/mvebu_defconfig
index a702fb3..cf597db 100644
--- a/arch/arm/configs/mvebu_defconfig
+++ b/arch/arm/configs/mvebu_defconfig
@@ -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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Jason Gunthorpe]

On Fri, Dec 07, 2012 at 11:04:23PM +0100, Thomas Petazzoni wrote:

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

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

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

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

Regards,
Jason

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Stephen Warren]

On 12/07/2012 04:33 PM, Jason Gunthorpe wrote:
> On Fri, Dec 07, 2012 at 11:04:23PM +0100, Thomas Petazzoni wrote:
> 
>>  * 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
> 
> 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.

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.

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thomas Petazzoni]

Dear Stephen Warren,

On Mon, 10 Dec 2012 10:52:33 -0700, Stephen Warren wrote:

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

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.

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

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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Stephen Warren]

On 12/10/2012 11:05 AM, Thomas Petazzoni wrote:
> Dear Stephen Warren,
> 
> On Mon, 10 Dec 2012 10:52:33 -0700, Stephen Warren wrote:
> 
>> 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.
> 
> 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.

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?

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thomas Petazzoni]

Dear Stephen Warren,

On Mon, 10 Dec 2012 11:16:04 -0700, Stephen Warren wrote:

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

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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Jason Gunthorpe]

On Mon, Dec 10, 2012 at 07:59:35PM +0100, Thomas Petazzoni wrote:

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

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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Stephen Warren]

On 12/10/2012 11:59 AM, Thomas Petazzoni wrote:
...
> 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
...
> 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

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

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

That sounds about right.

> Does that answer your question?

Yes, thanks.

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Jason Gunthorpe]

On Mon, Dec 10, 2012 at 10:52:33AM -0700, Stephen Warren wrote:
 
> 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.

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.

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

If you can't do full dynamic configuration, then I think at least the
allocation size per port needs to be in device tree.
 
> 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.

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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Mon, 10 Dec 2012 11:44:39 -0700, Jason Gunthorpe wrote:

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

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?

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

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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Jason Gunthorpe]

On Mon, Dec 10, 2012 at 08:03:50PM +0100, Thomas Petazzoni wrote:

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

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.

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

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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Mon, 10 Dec 2012 12:18:43 -0700, Jason Gunthorpe wrote:

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

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

> > 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.
> 
> Yes, though you might want to think about having the window numbers
> assigned staticly (for PCI-E and everything else) in device tree

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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Jason Gunthorpe]

On Wed, Dec 12, 2012 at 05:04:17PM +0100, Thomas Petazzoni wrote:
> Dear Jason Gunthorpe,
> 
> On Mon, 10 Dec 2012 12:18:43 -0700, Jason Gunthorpe wrote:
> 
> > 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.
> 
> So I should implement fake PCI configuration read/write operations, and
> emulate a PCIe bridge? Sounds complicated...

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.

> > > 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.
> > 
> > Yes, though you might want to think about having the window numbers
> > assigned staticly (for PCI-E and everything else) in device tree
> 
> 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

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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

On Wed, Dec 12, 2012 at 01:09:10PM -0700, Jason Gunthorpe wrote:
> On Wed, Dec 12, 2012 at 05:04:17PM +0100, Thomas Petazzoni wrote:
> > Dear Jason Gunthorpe,
> > 
> > On Mon, 10 Dec 2012 12:18:43 -0700, Jason Gunthorpe wrote:
> > 
> > > 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.
> > 
> > So I should implement fake PCI configuration read/write operations, and
> > emulate a PCIe bridge? Sounds complicated...
> 
> 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..

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.

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

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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

On Mon, Dec 10, 2012 at 10:52:33AM -0700, Stephen Warren wrote:
> On 12/07/2012 04:33 PM, Jason Gunthorpe wrote:
> > On Fri, Dec 07, 2012 at 11:04:23PM +0100, Thomas Petazzoni wrote:
> > 
> >>  * 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
> > 
> > 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.
> 
> 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.

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.

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

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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Stephen Warren]

On 12/11/2012 12:52 AM, Thierry Reding wrote:
> On Mon, Dec 10, 2012 at 10:52:33AM -0700, Stephen Warren wrote:
>> On 12/07/2012 04:33 PM, Jason Gunthorpe wrote:
>>> On Fri, Dec 07, 2012 at 11:04:23PM +0100, Thomas Petazzoni
>>> wrote:
>>> 
>>>> * 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
>>> 
>>> 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.
>> 
>> 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.
> 
> 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.

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.

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

On Tue, Dec 11, 2012 at 02:21:09PM -0700, Stephen Warren wrote:
> On 12/11/2012 12:52 AM, Thierry Reding wrote:
> > On Mon, Dec 10, 2012 at 10:52:33AM -0700, Stephen Warren wrote:
> >> On 12/07/2012 04:33 PM, Jason Gunthorpe wrote:
> >>> On Fri, Dec 07, 2012 at 11:04:23PM +0100, Thomas Petazzoni
> >>> wrote:
> >>> 
> >>>> * 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
> >>> 
> >>> 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.
> >> 
> >> 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.
> > 
> > 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.
> 
> 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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Stephen Warren]

On 12/12/2012 01:34 PM, Thierry Reding wrote:
> On Tue, Dec 11, 2012 at 02:21:09PM -0700, Stephen Warren wrote:
>> On 12/11/2012 12:52 AM, Thierry Reding wrote:
...
>>> 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.
>> 
>> 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.

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?

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

On Wed, Dec 12, 2012 at 03:30:05PM -0700, Stephen Warren wrote:
> On 12/12/2012 01:34 PM, Thierry Reding wrote:
> > On Tue, Dec 11, 2012 at 02:21:09PM -0700, Stephen Warren wrote:
> >> On 12/11/2012 12:52 AM, Thierry Reding wrote:
> ...
> >>> 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.
> >> 
> >> 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.
> 
> 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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Jason Gunthorpe]

On Thu, Dec 13, 2012 at 08:03:32AM +0100, Thierry Reding wrote:

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

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.

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

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.

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

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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

On Thu, Dec 13, 2012 at 01:04:15AM -0700, Jason Gunthorpe wrote:
> On Thu, Dec 13, 2012 at 08:03:32AM +0100, Thierry Reding wrote:
> 
> > 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. 
> 
> 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.
> 
> > 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.
> 
> 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.
> 
> > 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.
> 
> 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.

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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Stephen Warren]

On 12/13/2012 01:23 AM, Thierry Reding wrote:
...
> 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.

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.

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

On Thu, Dec 13, 2012 at 11:12:29AM -0700, Stephen Warren wrote:
> On 12/13/2012 01:23 AM, Thierry Reding wrote:
> ...
> > 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.
> 
> 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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Jason Gunthorpe]

On Thu, Dec 13, 2012 at 09:42:29PM +0100, Thierry Reding wrote:

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

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

Jason

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

On Thu, Dec 13, 2012 at 01:47:18PM -0700, Jason Gunthorpe wrote:
> On Thu, Dec 13, 2012 at 09:42:29PM +0100, Thierry Reding wrote:
> 
> > 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.
> 
> With the special case, what does device 0:0.0 show up as? What class?

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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

On Thu, Dec 13, 2012 at 01:47:18PM -0700, Jason Gunthorpe wrote:
> On Thu, Dec 13, 2012 at 09:42:29PM +0100, Thierry Reding wrote:
> 
> > 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.
> 
> With the special case, what does device 0:0.0 show up as? What class?

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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

On Thu, Dec 13, 2012 at 09:42:29PM +0100, Thierry Reding wrote:
> On Thu, Dec 13, 2012 at 11:12:29AM -0700, Stephen Warren wrote:
> > On 12/13/2012 01:23 AM, Thierry Reding wrote:
> > ...
> > > 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.
> > 
> > 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.

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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Jason Gunthorpe]

On Fri, Dec 14, 2012 at 04:10:45PM +0100, Thierry Reding wrote:
> > 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.
> 
> 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

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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

On Fri, Dec 14, 2012 at 10:27:29AM -0700, Jason Gunthorpe wrote:
> On Fri, Dec 14, 2012 at 04:10:45PM +0100, Thierry Reding wrote:
> > > 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.
> > 
> > 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
> 
> This is your 
> 
>         02:00.0 PCI bridge: Avionic Design GmbH FPGA PCIe PCI-to-PCI (P2P) Bridge
> 
> Device, right?

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.

> 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

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.

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

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.

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

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.

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

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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Jason Gunthorpe]

> > Ie route access for 00:1N.0 to the configuration space on bridge port N
> 
> 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.

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.

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

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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

On Mon, Dec 17, 2012 at 11:29:11AM -0700, Jason Gunthorpe wrote:
> 
> > > Ie route access for 00:1N.0 to the configuration space on bridge port N
> > 
> > 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.
> 
> 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.

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.

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

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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Stephen Warren]

On 12/17/2012 12:41 PM, Thierry Reding wrote:
> On Mon, Dec 17, 2012 at 11:29:11AM -0700, Jason Gunthorpe wrote:
>> 
>>>> Ie route access for 00:1N.0 to the configuration space on
>>>> bridge port N
>>> 
>>> 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.
>> 
>> 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.
> 
> 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.

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.

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Jason Gunthorpe]

[trimmed the cc list]

On Mon, Dec 17, 2012 at 07:10:59PM -0700, Stephen Warren wrote:

> > 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.
> 
> Some internal discussion implies this shouldn't be required; here's
> what they said:

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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Stephen Warren]

On 12/17/2012 07:51 PM, Jason Gunthorpe wrote:
> [trimmed the cc list]
> 
> On Mon, Dec 17, 2012 at 07:10:59PM -0700, Stephen Warren wrote:
> 
>>> 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.
>>
>> Some internal discussion implies this shouldn't be required; here's
>> what they said:
> 
> 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?

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.

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

On Tue, Dec 18, 2012 at 10:03:38AM -0700, Stephen Warren wrote:
> On 12/17/2012 07:51 PM, Jason Gunthorpe wrote:
> > [trimmed the cc list]
> > 
> > On Mon, Dec 17, 2012 at 07:10:59PM -0700, Stephen Warren wrote:
> > 
> >>> 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.
> >>
> >> Some internal discussion implies this shouldn't be required; here's
> >> what they said:
> > 
> > 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?
> 
> 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).

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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Jay Agarwal]

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.



> -----Original Message-----

> From: Thierry Reding [mailto:thierry.reding at avionic-design.de]

> Sent: Thursday, December 20, 2012 9:03 PM

> To: Stephen Warren

> Cc: Jason Gunthorpe; Jay Agarwal; Thomas Petazzoni; Andrew Lunn; Jason

> Cooper; Gregory Clement; linux-arm-kernel at lists.infradead.org

> Subject: Re: [RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

>

> * PGP Signed by an unknown key

>

> On Tue, Dec 18, 2012 at 10:03:38AM -0700, Stephen Warren wrote:

> > On 12/17/2012 07:51 PM, Jason Gunthorpe wrote:

> > > [trimmed the cc list]

> > >

> > > On Mon, Dec 17, 2012 at 07:10:59PM -0700, Stephen Warren wrote:

> > >

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

> > >>

> > >> Some internal discussion implies this shouldn't be required; here's

> > >> what they said:

> > >

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

> >

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

>

> 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

>

> * Unknown Key

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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

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

On Fri, Dec 21, 2012 at 07:08:39PM +0530, Jay Agarwal wrote:
> 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.

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.

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

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

> 
> 
> 
> > -----Original Message-----
> 
> > From: Thierry Reding [mailto:thierry.reding at avionic-design.de]
> 
> > Sent: Thursday, December 20, 2012 9:03 PM
> 
> > To: Stephen Warren
> 
> > Cc: Jason Gunthorpe; Jay Agarwal; Thomas Petazzoni; Andrew Lunn; Jason
> 
> > Cooper; Gregory Clement; linux-arm-kernel at lists.infradead.org
> 
> > Subject: Re: [RFC v1] PCIe support for the Armada 370 and Armada XP SoCs
> 
> >
> 
> > * PGP Signed by an unknown key
> 
> >
> 
> > On Tue, Dec 18, 2012 at 10:03:38AM -0700, Stephen Warren wrote:
> 
> > > On 12/17/2012 07:51 PM, Jason Gunthorpe wrote:
> 
> > > > [trimmed the cc list]
> 
> > > >
> 
> > > > On Mon, Dec 17, 2012 at 07:10:59PM -0700, Stephen Warren wrote:
> 
> > > >
> 
> > > >>> 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.
> 
> > > >>
> 
> > > >> Some internal discussion implies this shouldn't be required; here's
> 
> > > >> what they said:
> 
> > > >
> 
> > > > 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?
> 
> > >
> 
> > > 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).
> 
> >
> 
> > 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
> 
> >
> 
> > * Unknown Key
> 
> > * 0x7F3EB3A1
-------------- 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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thomas Petazzoni]

Dear Thierry Reding,

On Thu, 20 Dec 2012 16:32:31 +0100, Thierry Reding wrote:

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

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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

On Sat, Dec 22, 2012 at 03:50:40PM +0100, Thomas Petazzoni wrote:
> Dear Thierry Reding,
> 
> On Thu, 20 Dec 2012 16:32:31 +0100, Thierry Reding wrote:
> 
> > 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.
> 
> 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.

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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thomas Petazzoni]

Dear Thierry Reding,

On Fri, 28 Dec 2012 22:06:22 +0100, Thierry Reding wrote:

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

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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Stephen Warren]

On 12/28/2012 02:16 PM, Thomas Petazzoni wrote:
> Dear Thierry Reding,
> 
> On Fri, 28 Dec 2012 22:06:22 +0100, Thierry Reding wrote:
> 
>>> 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.
>>
>> 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.
> 
> 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.

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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thomas Petazzoni]

Dear Stephen Warren,

On Fri, 28 Dec 2012 16:49:15 -0700, Stephen Warren wrote:

> >> 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.
> > 
> > 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.
> 
> Is it actually necessary for a host/PCI bridge to show up on the PCI bus
> as device 0?

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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Stephen Warren]

On 12/29/2012 01:09 AM, Thomas Petazzoni wrote:
> Dear Stephen Warren,
> 
> On Fri, 28 Dec 2012 16:49:15 -0700, Stephen Warren wrote:
> 
>>>> 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.
>>>
>>> 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.
>>
>> Is it actually necessary for a host/PCI bridge to show up on the PCI bus
>> as device 0?
> 
> 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 ?

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.

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

On Fri, Dec 28, 2012 at 04:49:15PM -0700, Stephen Warren wrote:
> On 12/28/2012 02:16 PM, Thomas Petazzoni wrote:
> > Dear Thierry Reding,
> > 
> > On Fri, 28 Dec 2012 22:06:22 +0100, Thierry Reding wrote:
> > 
> >>> 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.
> >>
> >> 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.
> > 
> > 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.
> 
> Is it actually necessary for a host/PCI bridge to show up on the PCI bus
> as device 0?

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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Stephen Warren]

On 12/29/2012 02:33 AM, Thierry Reding wrote:
> On Fri, Dec 28, 2012 at 04:49:15PM -0700, Stephen Warren wrote:
...
>> Is it actually necessary for a host/PCI bridge to show up on the
>> PCI bus as device 0?
> 
> 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.

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.

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Jason Gunthorpe]

On Sat, Dec 29, 2012 at 10:33:59AM +0100, Thierry Reding wrote:

> 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

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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

On Wed, Jan 02, 2013 at 01:09:32PM -0700, Jason Gunthorpe wrote:
> On Sat, Dec 29, 2012 at 10:33:59AM +0100, Thierry Reding wrote:
> 
> > 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
> 
> Can you elaborate on this a bit? What do pci bus location bindings look
> like?

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.

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

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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Stephen Warren]

On 12/20/2012 08:32 AM, Thierry Reding wrote:
...
> 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?

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.

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

On Mon, Dec 17, 2012 at 07:10:59PM -0700, Stephen Warren wrote:
> On 12/17/2012 12:41 PM, Thierry Reding wrote:
> > On Mon, Dec 17, 2012 at 11:29:11AM -0700, Jason Gunthorpe wrote:
> >> 
> >>>> Ie route access for 00:1N.0 to the configuration space on
> >>>> bridge port N
> >>> 
> >>> 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.
> >> 
> >> 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.
> > 
> > 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.
> 
> 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.

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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Bjorn Helgaas]

On Thu, Jan 3, 2013 at 7:39 AM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> 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?

Sounds fine to me.

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

On Thu, Jan 03, 2013 at 08:00:58AM -0700, Bjorn Helgaas wrote:
> On Thu, Jan 3, 2013 at 7:39 AM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> > 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?
> 
> 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>

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thomas Petazzoni]

Dear Thierry Reding,

On Thu, 3 Jan 2013 15:39:31 +0100, Thierry Reding wrote:

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

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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Arnd Bergmann]

On Thursday 03 January 2013, Thierry Reding wrote:
> Stephen proposed drivers/pci/host on IRC and I second that.

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

[RFC v1] PCIe support for the Armada 370 and Armada XP SoCs

[Thierry Reding]

On Thu, Jan 03, 2013 at 03:56:31PM +0000, Arnd Bergmann wrote:
> On Thursday 03 January 2013, Thierry Reding wrote:
> > Stephen proposed drivers/pci/host on IRC and I second that.
> 
> 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.

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>