On 06/12/2012 11:20 AM, Thierry Reding wrote:
...

I came up with the following alternative:

	pci {
		compatible = "nvidia,tegra20-pcie";
		reg = <0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200   /* AFI registers */
		       0x80004000 0x00100000   /* configuration space */
		       0x80104000 0x00100000   /* extended configuration space */
		       0x80400000 0x00010000   /* downstream I/O */
		       0x90000000 0x10000000   /* non-prefetchable memory */
		       0xa0000000 0x10000000>; /* prefetchable memory */
		interrupts = <0 98 0x04   /* controller interrupt */
		              0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges = <0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80004000 0x80004000 0x00100000   /* configuration space */
			  0x80104000 0x80104000 0x00100000   /* extended configuration space */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells = <1>;
		#size-cells = <1>;

		port@80000000 {
			reg = <0x80000000 0x00001000>;
			status = "disabled";
		};

		port@80001000 {
			reg = <0x80001000 0x00001000>;
			status = "disabled";
		};
	};

The "ranges" property can probably be cleaned up a bit, but the most
interesting part is the port@ children, which can simply be enabled in board
DTS files by setting the status property to "okay". I find that somewhat more
intuitive to the variant with an "enable-ports" property.

What do you think of this?

As a general concept, this kind of design seems OK to me.

The "port" child nodes I think should be named "pci@..." given Mitch's
comments, I think.

The port nodes probably need two entries in reg, given the following in
our downstream driver:

        int rp_offset = 0;
        int ctrl_offset = AFI_PEX0_CTRL;

...

        for (port = 0; port < MAX_PCIE_SUPPORTED_PORTS; port++) {
                ctrl_offset += (port * 8);
                rp_offset = (rp_offset + 0x1000) * port;
                if (tegra_pcie.plat_data->port_status[port])
                        tegra_pcie_add_port(port, rp_offset, ctrl_offset);
        }

(which actually looks likely to be horribly buggy for port>1 and only
accidentally correct for port==1, but anyway...)

But instead, I'd be tempted to make the top-level node say:

	#address-cells = <1>;
	#size-cells = <0>;

... so that the child nodes' reg is just the port ID. The parent node
can calculate the addresses/offsets of the per-port registers within the
PCIe controller's register space based on the ID using code roughly like
what I quoted above:

	pci@0 {
		reg = <0>;
		status = "disabled";
	};

	pci@1 {
		reg = <0>;
		status = "disabled";
	};

That would save having to put 2 entries in the reg, and perhaps remove
the need for any ranges property.

I think you also need a property to specify the exact port layout; the
Tegra20 controller supports:

1 x4 port
2 x2 ports (you can choose to use only 1 of these I assume)

So just because only 1 of the ports is enabled, doesn't imply it's x4;
it could still be x2.

Tegra30 has more options.