[PATCH 6/6] ARM: dts: Add pcie controller node for Samsung EXYNOS5440 SoC

[Jason Gunthorpe]

On Wed, Mar 27, 2013 at 05:35:48PM +0900, Jingoo Han wrote:

> Here is the lspci -vv output.
> I tested Exynos PCIe with e1000e lan card.
> 
> 00:00.0 PCI bridge: Samsung Electronics Co Ltd Device a549 (rev 01) (prog-if 00 [Normal decode])
>         Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr+ Stepping- SERR+ FastB2B- DisINTx-
>         Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
>         Latency: 0, Cache Line Size: 64 bytes
>         Bus: primary=00, secondary=01, subordinate=01, sec-latency=0
>         I/O behind bridge: 00000000-00000fff
>         Memory behind bridge: 40300000-403fffff
>         Prefetchable memory behind bridge: 40400000-404fffff
>         Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR-
>         BridgeCtl: Parity+ SERR- NoISA- VGA- MAbort- >Reset- FastB2B-
>                 PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
>         Capabilities: <access denied>
>         Kernel driver in use: pcieport

This is very similar to tegra, you should try to follow the same path
as Thierry did.

Basically, have only one PCI host from Linux's perspective. This means
the driver only calls pci_create_root_bus once. The driver makes all
the ports available under that single root_bus. It does this by
routing the config access to the four different MMIO config regions
depending on the bus number and device number.

When done, lspci should look something like this:

 00:01.0 PCI bridge: Samsung Electronics Co Ltd Device a549 (rev 01) (prog-if 00 [Normal decode])
 00:02.0 PCI bridge: Samsung Electronics Co Ltd Device a549 (rev 01) (prog-if 00 [Normal decode])

Notice the bus number of both root port bridges is 0. This is now
compliant with the PCI-E specification for root complex behavior. Bus
0 is the root complex bus.

The driver can then use this information in the bridges:
>         Bus: primary=00, secondary=01, subordinate=01, sec-latency=0
To route config transactions for bus != 0 to the proper port and
correctly generate type 0 or type 1 config TLPs.

I hope this makes sense. Tegra's implementation is very close to this,
but the bus != 0 case will be more like Marvell. (If I read your
driver right)

> 10:00.0 PCI bridge: Samsung Electronics Co Ltd Device a549 (rev 01)
> (prog-if 00 [Normal decode])

There is something funny here, this is bus 0x10, but your bindings had
bus-range 0 -> 0xf on both nodes.

> > However, based on your driver this HW looks similar to tegra, did you
> > review how tegra is setup? Merging all the ports into a single domain
> > is certainly preferred.
> 
> In Tegra case, the address of IO control register is same.
> +	pcie-controller {
> +		compatible = "nvidia,tegra20-pcie";
> +		reg = <0x80003000 0x00000800   /* PADS registers */
> +		       0x80003800 0x00000200   /* AFI registers */
> +		       0x81000000 0x01000000   /* configuration space */
> +		       0x90000000 0x10000000>; /* extended configuration space */
>
> But, in Exynos case, address of IP control register is different
> between PCIe0 and PCIe1.

tegra has both shared registers and per-port registers. The ones above
are shared.

This message has the final DT bindings for the Marvell and tegra
cases:

http://permalink.gmane.org/gmane.linux.kernel.pci/21209

The per-port registers are being passed to the driver here:

   	     pci at 1,0 {
 	     	      device_type = "pci";
 		      assigned-addresses = <0x82000800 0 0x80000000 0 0x1000>;
                      reg = <0x000800 0 0 0 0>;

via assigned-addresses.

Marvell has no shared registers, you can see in its binding they are
all passed through assigned-addresses.

Also, the above DT nodes is representing the root port bridge PCI
device. In your case it would be this:

 00:01.0 PCI bridge: Samsung Electronics Co Ltd Device a549 (rev 01) (prog-if 00 [Normal decode])

0x800 is the DT encoding of 00:01.0

> > > +		ranges = <0x00000800 0 0x60000000 0x60000000 0 0x00200000   /* configuration space */
> > 
> > Do not include configuration space in ranges
> 
> How can I include configuration space?
> Please let me know kindly :)

There is no need to specify configuration space at all. If you copied
this from an older tegra binding it was an early way to try and define
per-port registers. After discussion the use of assigned-addresses was
choosen instead.
 
> > It is usual to have an interrupt-map - have you tested that interrupts
> > resolve properly?
>
> There is no problem about interrupts.

I see, you have exynos_pcie_map_irq in code. interrupt-map replaces
that functionality in a standard way, and is more capable for edge
cases.

> However, I will consider an interrupt-map.

You can copy the interrupt-map style from the Marvell driver, which
seems like it matches your case..

Jason