diff mbox

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

Message ID 00c501ce277c$30e49dc0$92add940$%han@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jingoo Han March 23, 2013, 4:09 a.m. UTC
Exynos5440 has two PCIe controllers which can be used as root complex
for PCIe interface.

Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
 arch/arm/boot/dts/exynos5440-ssdk5440.dts |    8 +++++++
 arch/arm/boot/dts/exynos5440.dtsi         |   32 +++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 0 deletions(-)

Comments

Jason Gunthorpe March 25, 2013, 5:04 p.m. UTC | #1
On Sat, Mar 23, 2013 at 01:09:18PM +0900, Jingoo Han wrote:

> +	pcie0@40000000 {
> +		compatible = "samsung,exynos5440-pcie";
> +		reg = <0x40000000 0x4000
> +			0x290000 0x1000
> +			0x270000 0x1000
> +			0x271000 0x40>;
> +		interrupts = <0 20 0>, <0 21 0>, <0 22 0>;
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		device_type = "pci";
> +		bus-range = <0x0 0xf>;
> +		ranges = <0x00000800 0 0x40000000 0x40000000 0 0x00200000   /* configuration space */
> +			  0x81000000 0 0	  0x40200000 0 0x00004000   /* downstream I/O */
> +			  0x82000000 0 0	  0x40204000 0 0x10000000>; /* non-prefetchable memory */
> +	};

Can you send the lspci output so these bindings can be properly
reviewed? What PCI devices are internal to the SOC?

What is behind 'exynos_pcie_wr_own_conf' ? Is this a root port bridge
config space? What line is it in the lspci output? Can you include a
lspci -vv for it as well?

Your DT has overlapping bus-ranges, and two top level nodes. This is
going to require separate PCI domains in Linux.

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.

> +	pcie1@60000000 {
> +		compatible = "samsung,exynos5440-pcie";
> +		reg = <0x60000000 0x4000
> +			0x2a0000 0x1000
> +			0x272000 0x1000
> +			0x271040 0x40>;
> +		interrupts = <0 23 0>, <0 24 0>, <0 25 0>;
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		device_type = "pci";
> +		bus-range = <0x0 0xf>;
> +		ranges = <0x00000800 0 0x60000000 0x60000000 0 0x00200000   /* configuration space */

Do not include configuration space in ranges

> +			  0x81000000 0 0	  0x60200000 0 0x00004000   /* downstream I/O */

Please confirm that an MMIO to 0x60200000 produces a PCI-E IO TLP to
address 0

> +			  0x82000000 0 0	  0x60204000 0 0x10000000>; /* non-prefetchable memory */

Please check this, generally it should be:

			  0x82000000 0 0x60204000 0x60204000 0 0x10000000>; /* non-prefetchable memory */

Reflecting an identity mapping for MMIO - eg MMIO access to 0x60204000
producse a PCI Memory TLP to address 0x60204000 - unless your hardware
is actually doing address translation (then there are other things to
confirm..)

It is usual to have an interrupt-map - have you tested that interrupts
resolve properly?

It looks like the INTx's should be routed by an interrupt-map to the
pulse pin. Consider an interrupt controller to decode the INT ABCD.

Regards,
Jason
Jingoo Han March 27, 2013, 8:35 a.m. UTC | #2
On Tuesday, March 26, 2013 2:05 AM, Jason Gunthorpe wrote:
> 
> On Sat, Mar 23, 2013 at 01:09:18PM +0900, Jingoo Han wrote:
> 
> > +	pcie0@40000000 {
> > +		compatible = "samsung,exynos5440-pcie";
> > +		reg = <0x40000000 0x4000
> > +			0x290000 0x1000
> > +			0x270000 0x1000
> > +			0x271000 0x40>;
> > +		interrupts = <0 20 0>, <0 21 0>, <0 22 0>;
> > +		#address-cells = <3>;
> > +		#size-cells = <2>;
> > +		device_type = "pci";
> > +		bus-range = <0x0 0xf>;
> > +		ranges = <0x00000800 0 0x40000000 0x40000000 0 0x00200000   /* configuration space */
> > +			  0x81000000 0 0	  0x40200000 0 0x00004000   /* downstream I/O */
> > +			  0x82000000 0 0	  0x40204000 0 0x10000000>; /* non-prefetchable memory */
> > +	};
> 
> Can you send the lspci output so these bindings can be properly
> reviewed? What PCI devices are internal to the SOC?
> 
> What is behind 'exynos_pcie_wr_own_conf' ? Is this a root port bridge
> config space? What line is it in the lspci output? Can you include a
> lspci -vv for it as well?

Hi Jason Gunthorpe,
Thank you for your comment :)

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

01:00.0 Ethernet controller: Intel Corporation 82574L Gigabit Network Connection
        Subsystem: Intel Corporation Gigabit CT Desktop Adapter
        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
        Interrupt: pin A routed to IRQ 53
        Region 0: Memory at 40380000 (32-bit, non-prefetchable) [size=128K]
        Region 1: Memory at 40300000 (32-bit, non-prefetchable) [size=512K]
        Region 2: I/O ports at 40200000 [disabled] [size=32]
        Region 3: Memory at 403a0000 (32-bit, non-prefetchable) [size=16K]
        [virtual] Expansion ROM at 40400000 [disabled] [size=256K]
        Capabilities: <access denied>
        Kernel driver in use: e1000e

10: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=10, secondary=11, subordinate=11, sec-latency=0
        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



> 
> Your DT has overlapping bus-ranges, and two top level nodes. This is
> going to require separate PCI domains in Linux.
> 
> 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.

+	pcie0@40000000 {
+		compatible = "samsung,exynos5440-pcie";
+		reg = <0x40000000 0x4000
+			0x290000 0x1000
+			0x270000 0x1000
+			0x271000 0x40>;

+	pcie1@60000000 {
+		compatible = "samsung,exynos5440-pcie";
+		reg = <0x60000000 0x4000
+			0x2a0000 0x1000
+			0x272000 0x1000
+			0x271040 0x40>;


> 
> > +	pcie1@60000000 {
> > +		compatible = "samsung,exynos5440-pcie";
> > +		reg = <0x60000000 0x4000
> > +			0x2a0000 0x1000
> > +			0x272000 0x1000
> > +			0x271040 0x40>;
> > +		interrupts = <0 23 0>, <0 24 0>, <0 25 0>;
> > +		#address-cells = <3>;
> > +		#size-cells = <2>;
> > +		device_type = "pci";
> > +		bus-range = <0x0 0xf>;
> > +		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 :)

> 
> > +			  0x81000000 0 0	  0x60200000 0 0x00004000   /* downstream I/O */
> 
> Please confirm that an MMIO to 0x60200000 produces a PCI-E IO TLP to
> address 0
> 
> > +			  0x82000000 0 0	  0x60204000 0 0x10000000>; /* non-prefetchable memory */
> 
> Please check this, generally it should be:
> 
> 			  0x82000000 0 0x60204000 0x60204000 0 0x10000000>; /* non-prefetchable memory */
> 
> Reflecting an identity mapping for MMIO - eg MMIO access to 0x60204000
> producse a PCI Memory TLP to address 0x60204000 - unless your hardware
> is actually doing address translation (then there are other things to
> confirm..)

OK, I will change it.

> 
> It is usual to have an interrupt-map - have you tested that interrupts
> resolve properly?

There is no problem about interrupts.
However, I will consider an interrupt-map.

> 
> It looks like the INTx's should be routed by an interrupt-map to the
> pulse pin. Consider an interrupt controller to decode the INT ABCD.
> 
> Regards,
> Jason
Jason Gunthorpe March 27, 2013, 4:13 p.m. UTC | #3
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@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
Jingoo Han April 8, 2013, 9:08 a.m. UTC | #4
On Saturday, March 23, 2013 1:09 PM, Jingoo Han wrote:
> 
> Exynos5440 has two PCIe controllers which can be used as root complex
> for PCIe interface.
> 
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>
> ---
>  arch/arm/boot/dts/exynos5440-ssdk5440.dts |    8 +++++++
>  arch/arm/boot/dts/exynos5440.dtsi         |   32 +++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/exynos5440-ssdk5440.dts b/arch/arm/boot/dts/exynos5440-ssdk5440.dts
> index a21eb4c..746f9fc 100644
> --- a/arch/arm/boot/dts/exynos5440-ssdk5440.dts
> +++ b/arch/arm/boot/dts/exynos5440-ssdk5440.dts
> @@ -34,4 +34,12 @@
>  			clock-frequency = <50000000>;
>  		};
>  	};
> +
> +	pcie0@40000000 {
> +		reset-gpio = <5>;
> +	};
> +
> +	pcie1@60000000 {
> +		reset-gpio = <22>;
> +	};
>  };
> diff --git a/arch/arm/boot/dts/exynos5440.dtsi b/arch/arm/boot/dts/exynos5440.dtsi
> index c374a31..41b2d2c 100644
> --- a/arch/arm/boot/dts/exynos5440.dtsi
> +++ b/arch/arm/boot/dts/exynos5440.dtsi
> @@ -178,4 +178,36 @@
>  		clocks = <&clock 21>;
>  		clock-names = "rtc";
>  	};
> +
> +	pcie0@40000000 {
> +		compatible = "samsung,exynos5440-pcie";
> +		reg = <0x40000000 0x4000
> +			0x290000 0x1000
> +			0x270000 0x1000
> +			0x271000 0x40>;
> +		interrupts = <0 20 0>, <0 21 0>, <0 22 0>;
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		device_type = "pci";
> +		bus-range = <0x0 0xf>;
> +		ranges = <0x00000800 0 0x40000000 0x40000000 0 0x00200000   /* configuration space */
> +			  0x81000000 0 0	  0x40200000 0 0x00004000   /* downstream I/O */
> +			  0x82000000 0 0	  0x40204000 0 0x10000000>; /* non-prefetchable memory */
> +	};
> +
> +	pcie1@60000000 {
> +		compatible = "samsung,exynos5440-pcie";
> +		reg = <0x60000000 0x4000
> +			0x2a0000 0x1000
> +			0x272000 0x1000
> +			0x271040 0x40>;
> +		interrupts = <0 23 0>, <0 24 0>, <0 25 0>;
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		device_type = "pci";
> +		bus-range = <0x0 0xf>;
> +		ranges = <0x00000800 0 0x60000000 0x60000000 0 0x00200000   /* configuration space */
> +			  0x81000000 0 0	  0x60200000 0 0x00004000   /* downstream I/O */
> +			  0x82000000 0 0	  0x60204000 0 0x10000000>; /* non-prefetchable memory */
> +	};

Hi Jason,

I have a question.
Now, I am reviewing the Tegra PCIe, Marvell PCIe patchset.
However, in the case of Exynos PCIe,
'downstream I/O' and 'non-prefetchable memory' are different between PCIe0 and PCIe1.
These regions are not shared.

PCIe0:
	ranges = <0x00000800 0 0x40000000 0x40000000 0 0x00200000   /* configuration space */
		  0x81000000 0 0	  0x40200000 0 0x00004000   /* downstream I/O */
		  0x82000000 0 0	  0x40204000 0 0x10000000>; /* non-prefetchable memory */

PCIe1:
	ranges = <0x00000800 0 0x40000000 0x40000000 0 0x00200000   /* configuration space */
		  0x81000000 0 0	  0x40200000 0 0x00004000   /* downstream I/O */
		  0x82000000 0 0	  0x40204000 0 0x10000000>; /* non-prefetchable memory */

PCIe0 uses 0x40000000~0x5fffffff, PCI1 uses 0x60000000~0x7fffffff.

How can I handle this? :)
The following is right?

+       pcie-controller {
		.....
+               ranges = <0x82000000 0 0x40000000 0x40000000 0 0x00200000   /* port 0 registers */
+                         0x82000000 0 0x60000000 0x60000000 0 0x00200000   /* port 1 registers */
+                         0x81000000 0 0          0x40200000 0 0x00004000   /* port 0 downstream I/O */
+                         0x81000000 0 0          0x60200000 0 0x00004000   /* port 1 downstream I/O */
+                         0x82000000 0 0x40204000 0x40204000 0 0x10000000>; /* port 0 non-prefetchable memory */
+                         0x82000000 0 0x40204000 0x60204000 0 0x10000000>; /* port 1 non-prefetchable memory */
+
+               pci@1,0 {
+                       device_type = "pci";
+                       assigned-addresses = <0x82000800 0 0x40000000 0 0x00200000
+                                                 0x81000800 0 0x40200000 0 0x00004000
+                                                 0x81000800 0 0x40204000 0 0x10000000>;
		.....
+               pci@2,0 {
+                       device_type = "pci";
+                       assigned-addresses = <0x82000800 0 0x60000000 0 0x00200000
+                                                 0x81000800 0 0x60200000 0 0x00004000
+                                                 0x81000800 0 0x60204000 0 0x10000000>;

Best regards,
Jingoo Han


>  };
> --
> 1.7.2.5
Jason Gunthorpe April 8, 2013, 4:56 p.m. UTC | #5
On Mon, Apr 08, 2013 at 06:08:53PM +0900, Jingoo Han wrote:

> I have a question.  Now, I am reviewing the Tegra PCIe, Marvell PCIe
> patchset.  However, in the case of Exynos PCIe, 'downstream I/O' and
> 'non-prefetchable memory' are different between PCIe0 and PCIe1.
> These regions are not shared.
> 
> PCIe0:
> 	ranges = <0x00000800 0 0x40000000 0x40000000 0 0x00200000   /* configuration space */
> 		  0x81000000 0 0	  0x40200000 0 0x00004000   /* downstream I/O */
> 		  0x82000000 0 0	  0x40204000 0 0x10000000>; /* non-prefetchable memory */
> 
> PCIe1:
> 	ranges = <0x00000800 0 0x40000000 0x40000000 0 0x00200000   /* configuration space */
> 		  0x81000000 0 0	  0x40200000 0 0x00004000   /* downstream I/O */
> 		  0x82000000 0 0	  0x40204000 0 0x10000000>; /* non-prefetchable memory */
> 
> PCIe0 uses 0x40000000~0x5fffffff, PCI1 uses 0x60000000~0x7fffffff.
> 
> How can I handle this? :)

You need to dig into where this range restriction comes from, and how
it interacts with the PCI-E root bridge's window registers. Is there
another set of registers that control this? Is it hardwired into the
silicon? Do the root port window registers control this? 

I'm looking at functions like exynos_pcie_prog_viewport_mem_outbound
and wondering if the driver already controls this window.. But it
looks like there may be some restrictions.

Marvell also has unshared regions, but the driver arranges for those
ranges to be setup dynamically based on writes to the bridge's window
registers from the Linux PCI core, so the region is always in sync
with what the Linux PCI core is trying to do.

The desired perfect outcome is to have a single logical 'shared'
region for memory and I/O - give that region to the PCI core via
struct resources, then the PCI core tells the driver and HW what
portion of that region belongs to each root port via a write to the
root port bridge's window registers. The net result is still
non-overlapping regions, but the allocation of space between port 0
and port 1 is performed at run time.

I don't really know enough about your hardware to give you better
advice, sorry. The general guidance to try and follow the PCI-E spec
for a root complex is good, but if the HW can't do it, or it would
make the driver too complex, then one PCI domain per port will always
work (this is similar to your original driver, but with domains).

The main advantage to following the PCI-E specs and allowing for
dynamic allocation of address space is that it lets you reserve less
address space for PCI-E, and this in turn gives you more low mem
address space to use for DRAM.

> The following is right?
 
> +       pcie-controller {
> 		.....
> +               ranges = <0x82000000 0 0x40000000 0x40000000 0 0x00200000   /* port 0 registers */
> +                         0x82000000 0 0x60000000 0x60000000 0 0x00200000   /* port 1 registers */
> +                         0x81000000 0 0          0x40200000 0 0x00004000   /* port 0 downstream I/O */
> +                         0x81000000 0 0          0x60200000 0 0x00004000   /* port 1 downstream I/O */
> +                         0x82000000 0 0x40204000 0x40204000 0 0x10000000>; /* port 0 non-prefetchable memory */
> +                         0x82000000 0 0x40204000 0x60204000 0 0x10000000>; /* port 1 non-prefetchable memory */


> +
> +               pci@1,0 {
> +                       device_type = "pci";
> +                       assigned-addresses = <0x82000800 0 0x40000000 0 0x00200000
> +                                                 0x81000800 0 0x40200000 0 0x00004000
> +                                                 0x81000800 0 0x40204000 0 0x10000000>;

Would be:

                       ranges = <0x81000800 0 0x40200000  0x81000800 0 0x40200000  0 0x00004000
                                 0x81000800 0 0x40204000  0x81000800 0 0x40204000  0 0x10000000>;
                       assigned-addresses = <0x82000800 0 0x40000000  0 0x00200000>;

Jason
Jingoo Han June 7, 2013, 9:19 a.m. UTC | #6
On Tuesday, April 09, 2013 1:56 AM, Jason Gunthorpe wrote:
> On Mon, Apr 08, 2013 at 06:08:53PM +0900, Jingoo Han wrote:
> 
> > I have a question.  Now, I am reviewing the Tegra PCIe, Marvell PCIe
> > patchset.  However, in the case of Exynos PCIe, 'downstream I/O' and
> > 'non-prefetchable memory' are different between PCIe0 and PCIe1.
> > These regions are not shared.
> >
> > PCIe0:
> > 	ranges = <0x00000800 0 0x40000000 0x40000000 0 0x00200000   /* configuration space */
> > 		  0x81000000 0 0	  0x40200000 0 0x00004000   /* downstream I/O */
> > 		  0x82000000 0 0	  0x40204000 0 0x10000000>; /* non-prefetchable memory */
> >
> > PCIe1:
> > 	ranges = <0x00000800 0 0x40000000 0x40000000 0 0x00200000   /* configuration space */
> > 		  0x81000000 0 0	  0x40200000 0 0x00004000   /* downstream I/O */
> > 		  0x82000000 0 0	  0x40204000 0 0x10000000>; /* non-prefetchable memory */
> >
> > PCIe0 uses 0x40000000~0x5fffffff, PCI1 uses 0x60000000~0x7fffffff.
> >
> > How can I handle this? :)
> 
> You need to dig into where this range restriction comes from, and how
> it interacts with the PCI-E root bridge's window registers. Is there
> another set of registers that control this? Is it hardwired into the
> silicon? Do the root port window registers control this?
> 
> I'm looking at functions like exynos_pcie_prog_viewport_mem_outbound
> and wondering if the driver already controls this window.. But it
> looks like there may be some restrictions.
> 
> Marvell also has unshared regions, but the driver arranges for those
> ranges to be setup dynamically based on writes to the bridge's window
> registers from the Linux PCI core, so the region is always in sync
> with what the Linux PCI core is trying to do.
> 
> The desired perfect outcome is to have a single logical 'shared'
> region for memory and I/O - give that region to the PCI core via
> struct resources, then the PCI core tells the driver and HW what
> portion of that region belongs to each root port via a write to the
> root port bridge's window registers. The net result is still
> non-overlapping regions, but the allocation of space between port 0
> and port 1 is performed at run time.
> 
> I don't really know enough about your hardware to give you better
> advice, sorry. The general guidance to try and follow the PCI-E spec
> for a root complex is good, but if the HW can't do it, or it would
> make the driver too complex, then one PCI domain per port will always
> work (this is similar to your original driver, but with domains).
> 
> The main advantage to following the PCI-E specs and allowing for
> dynamic allocation of address space is that it lets you reserve less
> address space for PCI-E, and this in turn gives you more low mem
> address space to use for DRAM.

Hi Jason Gunthorpe,

I implemented 'Single domain' with Exynos PCIe for last two months;
however, it cannot work properly due to the hardware restriction.
Each MEM region is hard-wired.

Thus, I will send Exynos PCIe V3 patch as 'Separate domains'.

Best regards,
Jingoo Han

> 
> > The following is right?
> 
> > +       pcie-controller {
> > 		.....
> > +               ranges = <0x82000000 0 0x40000000 0x40000000 0 0x00200000   /* port 0 registers */
> > +                         0x82000000 0 0x60000000 0x60000000 0 0x00200000   /* port 1 registers */
> > +                         0x81000000 0 0          0x40200000 0 0x00004000   /* port 0 downstream I/O */
> > +                         0x81000000 0 0          0x60200000 0 0x00004000   /* port 1 downstream I/O */
> > +                         0x82000000 0 0x40204000 0x40204000 0 0x10000000>; /* port 0 non-prefetchable
> memory */
> > +                         0x82000000 0 0x40204000 0x60204000 0 0x10000000>; /* port 1 non-prefetchable
> memory */
> 
> 
> > +
> > +               pci@1,0 {
> > +                       device_type = "pci";
> > +                       assigned-addresses = <0x82000800 0 0x40000000 0 0x00200000
> > +                                                 0x81000800 0 0x40200000 0 0x00004000
> > +                                                 0x81000800 0 0x40204000 0 0x10000000>;
> 
> Would be:
> 
>                        ranges = <0x81000800 0 0x40200000  0x81000800 0 0x40200000  0 0x00004000
>                                  0x81000800 0 0x40204000  0x81000800 0 0x40204000  0 0x10000000>;
>                        assigned-addresses = <0x82000800 0 0x40000000  0 0x00200000>;
> 
> Jason
Arnd Bergmann June 7, 2013, 11:59 a.m. UTC | #7
On Friday 07 June 2013 18:19:40 Jingoo Han wrote:
> Hi Jason Gunthorpe,
> 
> I implemented 'Single domain' with Exynos PCIe for last two months;
> however, it cannot work properly due to the hardware restriction.
> Each MEM region is hard-wired.
> 
> Thus, I will send Exynos PCIe V3 patch as 'Separate domains'.

Yes, I think that is best, if the hardware is clearly designed as
separate domains, this is what we should do by default in the
driver. For the Marvell case with its 10 separate ports, much
more address space would be wasted by having one domain per
port and that hardware let us work around it by remapping the
physical address space windows. For Exynos there is much less to
lose and I too cannot see how it would be done in the first
place.

	Arnd
Jason Gunthorpe June 7, 2013, 4:20 p.m. UTC | #8
On Fri, Jun 07, 2013 at 01:59:43PM +0200, Arnd Bergmann wrote:
> On Friday 07 June 2013 18:19:40 Jingoo Han wrote:
> > Hi Jason Gunthorpe,
> > 
> > I implemented 'Single domain' with Exynos PCIe for last two months;
> > however, it cannot work properly due to the hardware restriction.
> > Each MEM region is hard-wired.
> > 
> > Thus, I will send Exynos PCIe V3 patch as 'Separate domains'.
> 
> Yes, I think that is best, if the hardware is clearly designed as
> separate domains, this is what we should do by default in the
> driver. For the Marvell case with its 10 separate ports, much
> more address space would be wasted by having one domain per
> port and that hardware let us work around it by remapping the
> physical address space windows. For Exynos there is much less to
> lose and I too cannot see how it would be done in the first
> place.

Sounds fair to me.

But when we talk about multiple domains we don't mean a disjoint range
bus bus numbers, as your other email shows:

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

We mean multiple domains, it should look like this:

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

ie lspci -D.

Each domain gets a unique bus number range, config space, io range,
etc. This is much clearer to everyone than trying to pretend there is
only one domain when the HW is actually multi-domain.

Jason
Arnd Bergmann June 7, 2013, 5:43 p.m. UTC | #9
On Friday 07 June 2013, Jason Gunthorpe wrote:
> Sounds fair to me.
> 
> But when we talk about multiple domains we don't mean a disjoint range
> bus bus numbers, as your other email shows:
> 
> 00:00.0 PCI bridge: Samsung Electronics Co Ltd Device a549 (rev 01) (prog-if 00 [Normal decode])
> 10:00.0 PCI bridge: Samsung Electronics Co Ltd Device a549 (rev 01) (prog-if 00 [Normal decode])
> 
> We mean multiple domains, it should look like this:
> 
> 0000:00:00.0 PCI bridge: Samsung Electronics Co Ltd Device a549 (rev 01) (prog-if 00 [Normal decode])
> 0001:00:00.0 PCI bridge: Samsung Electronics Co Ltd Device a549 (rev 01) (prog-if 00 [Normal decode])
> 
> ie lspci -D.
> 
> Each domain gets a unique bus number range, config space, io range,
> etc. This is much clearer to everyone than trying to pretend there is
> only one domain when the HW is actually multi-domain.

Yes, absolutely. This means we also don't need a bus-range property in DT, since each
domain will allow all 255 buses.

	Arnd
Jingoo Han June 10, 2013, 8:38 a.m. UTC | #10
On Saturday, June 08, 2013 2:43 AM, Arnd Bergmann wrote:
> On Friday 07 June 2013, Jason Gunthorpe wrote:
> > Sounds fair to me.
> >
> > But when we talk about multiple domains we don't mean a disjoint range
> > bus bus numbers, as your other email shows:
> >
> > 00:00.0 PCI bridge: Samsung Electronics Co Ltd Device a549 (rev 01) (prog-if 00 [Normal decode])
> > 10:00.0 PCI bridge: Samsung Electronics Co Ltd Device a549 (rev 01) (prog-if 00 [Normal decode])
> >
> > We mean multiple domains, it should look like this:
> >
> > 0000:00:00.0 PCI bridge: Samsung Electronics Co Ltd Device a549 (rev 01) (prog-if 00 [Normal decode])
> > 0001:00:00.0 PCI bridge: Samsung Electronics Co Ltd Device a549 (rev 01) (prog-if 00 [Normal decode])
> >
> > ie lspci -D.
> >
> > Each domain gets a unique bus number range, config space, io range,
> > etc. This is much clearer to everyone than trying to pretend there is
> > only one domain when the HW is actually multi-domain.
> 
> Yes, absolutely. This means we also don't need a bus-range property in DT, since each
> domain will allow all 255 buses.

After removing a bus-range property in DT, it looks like:

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

For multiple domains, how can I fix the DT properties?

Current DT properties are as below:

+	pcie0@40000000 {
+		compatible = "samsung,exynos5440-pcie";
+		reg = <0x40000000 0x4000
+			0x290000 0x1000
+			0x270000 0x1000
+			0x271000 0x40>;
+		interrupts = <0 20 0>, <0 21 0>, <0 22 0>;
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		ranges = <0x00000800 0 0x40000000 0x40000000 0 0x00200000   /* configuration space */
+			  0x81000000 0 0	  0x40200000 0 0x00004000   /* downstream I/O */
+			  0x82000000 0 0	  0x40204000 0 0x10000000>; /* non-prefetchable memory */
+	};
+
+	pcie1@60000000 {
+		compatible = "samsung,exynos5440-pcie";
+		reg = <0x60000000 0x4000
+			0x2a0000 0x1000
+			0x272000 0x1000
+			0x271040 0x40>;
+		interrupts = <0 23 0>, <0 24 0>, <0 25 0>;
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		ranges = <0x00000800 0 0x60000000 0x60000000 0 0x00200000   /* configuration space */
+			  0x81000000 0 0	  0x60200000 0 0x00004000   /* downstream I/O */
+			  0x82000000 0 0	  0x60204000 0 0x10000000>; /* non-prefetchable memory */
+	};



Best regards,
Jingoo Han

> 
> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann June 10, 2013, 3:22 p.m. UTC | #11
On Monday 10 June 2013, Jingoo Han wrote:
> On Saturday, June 08, 2013 2:43 AM, Arnd Bergmann wrote:
> For multiple domains, how can I fix the DT properties?

Domains are a Linux concept, you have to pick a new domain number for each
struct hw_pci you register.
 
> Current DT properties are as below:
> 
> +	pcie0@40000000 {
> +		compatible = "samsung,exynos5440-pcie";
> +		reg = <0x40000000 0x4000
> +			0x290000 0x1000
> +			0x270000 0x1000
> +			0x271000 0x40>;
> +		interrupts = <0 20 0>, <0 21 0>, <0 22 0>;
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		device_type = "pci";
> +		ranges = <0x00000800 0 0x40000000 0x40000000 0 0x00200000   /* configuration space */
> +			  0x81000000 0 0	  0x40200000 0 0x00004000   /* downstream I/O */
> +			  0x82000000 0 0	  0x40204000 0 0x10000000>; /* non-prefetchable memory */
> +	};

An unrelated comment: your first "reg" field seems to overlap with part
of your configuration space. Is that intentional?

Also, shouldn't your memory space end on a 256MB boundary, rather than
extend up to 0x50203fff?

	Arnd
Jingoo Han June 11, 2013, 6 a.m. UTC | #12
On Tuesday, June 11, 2013 12:22 AM, Arnd Bergmann wrote:
> On Monday 10 June 2013, Jingoo Han wrote:
> > On Saturday, June 08, 2013 2:43 AM, Arnd Bergmann wrote:
> > For multiple domains, how can I fix the DT properties?
> 
> Domains are a Linux concept, you have to pick a new domain number for each
> struct hw_pci you register.

Hi Arnd,

Thank you for your reply.
It is very helpful. :)

I will set domain numbers for each struct hw_pci.

> 
> > Current DT properties are as below:
> >
> > +	pcie0@40000000 {
> > +		compatible = "samsung,exynos5440-pcie";
> > +		reg = <0x40000000 0x4000
> > +			0x290000 0x1000
> > +			0x270000 0x1000
> > +			0x271000 0x40>;
> > +		interrupts = <0 20 0>, <0 21 0>, <0 22 0>;
> > +		#address-cells = <3>;
> > +		#size-cells = <2>;
> > +		device_type = "pci";
> > +		ranges = <0x00000800 0 0x40000000 0x40000000 0 0x00200000   /* configuration space */
> > +			  0x81000000 0 0	  0x40200000 0 0x00004000   /* downstream I/O */
> > +			  0x82000000 0 0	  0x40204000 0 0x10000000>; /* non-prefetchable memory */
> > +	};
> 
> An unrelated comment: your first "reg" field seems to overlap with part
> of your configuration space. Is that intentional?

Yes, intentional.
But, I will try to remove it.

> 
> Also, shouldn't your memory space end on a 256MB boundary, rather than
> extend up to 0x50203fff?

According to the manual of Exynos PCIe, each memory space for Exynos PCIe
can support 512MB, including I/O, CFG regions.

Is there any problem when over 256MB boundary is used?
Please let me know. :)


Best regards,
Jingoo Han

> 
> 	Arnd
Arnd Bergmann June 12, 2013, 3:10 p.m. UTC | #13
On Tuesday 11 June 2013, Jingoo Han wrote:
> > > +           ranges = <0x00000800 0 0x40000000 0x40000000 0 0x00200000   /* configuration space */
> > > +                     0x81000000 0 0          0x40200000 0 0x00004000   /* downstream I/O */
> > > +                     0x82000000 0 0          0x40204000 0 0x10000000>; /* non-prefetchable memory */
> > > +   };
> > 
> ...
> > Also, shouldn't your memory space end on a 256MB boundary, rather than
> > extend up to 0x50203fff?
> 
> According to the manual of Exynos PCIe, each memory space for Exynos PCIe
> can support 512MB, including I/O, CFG regions.
> 
> Is there any problem when over 256MB boundary is used?
> Please let me know. :)

No, that's not a problem, but I think you should have the window span
the entire space that is provided in hardware. If there are 512 MB total, why
not use them?

You could use 

	ranges = <0x82000000 0 0          0x40204000 0 0x1fdfc000>;

to pass a range for the memory space that extends all the way until
0x5fffffff.

	Arnd
diff mbox

Patch

diff --git a/arch/arm/boot/dts/exynos5440-ssdk5440.dts b/arch/arm/boot/dts/exynos5440-ssdk5440.dts
index a21eb4c..746f9fc 100644
--- a/arch/arm/boot/dts/exynos5440-ssdk5440.dts
+++ b/arch/arm/boot/dts/exynos5440-ssdk5440.dts
@@ -34,4 +34,12 @@ 
 			clock-frequency = <50000000>;
 		};
 	};
+
+	pcie0@40000000 {
+		reset-gpio = <5>;
+	};
+
+	pcie1@60000000 {
+		reset-gpio = <22>;
+	};
 };
diff --git a/arch/arm/boot/dts/exynos5440.dtsi b/arch/arm/boot/dts/exynos5440.dtsi
index c374a31..41b2d2c 100644
--- a/arch/arm/boot/dts/exynos5440.dtsi
+++ b/arch/arm/boot/dts/exynos5440.dtsi
@@ -178,4 +178,36 @@ 
 		clocks = <&clock 21>;
 		clock-names = "rtc";
 	};
+
+	pcie0@40000000 {
+		compatible = "samsung,exynos5440-pcie";
+		reg = <0x40000000 0x4000
+			0x290000 0x1000
+			0x270000 0x1000
+			0x271000 0x40>;
+		interrupts = <0 20 0>, <0 21 0>, <0 22 0>;
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		bus-range = <0x0 0xf>;
+		ranges = <0x00000800 0 0x40000000 0x40000000 0 0x00200000   /* configuration space */
+			  0x81000000 0 0	  0x40200000 0 0x00004000   /* downstream I/O */
+			  0x82000000 0 0	  0x40204000 0 0x10000000>; /* non-prefetchable memory */
+	};
+
+	pcie1@60000000 {
+		compatible = "samsung,exynos5440-pcie";
+		reg = <0x60000000 0x4000
+			0x2a0000 0x1000
+			0x272000 0x1000
+			0x271040 0x40>;
+		interrupts = <0 23 0>, <0 24 0>, <0 25 0>;
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		bus-range = <0x0 0xf>;
+		ranges = <0x00000800 0 0x60000000 0x60000000 0 0x00200000   /* configuration space */
+			  0x81000000 0 0	  0x60200000 0 0x00004000   /* downstream I/O */
+			  0x82000000 0 0	  0x60204000 0 0x10000000>; /* non-prefetchable memory */
+	};
 };