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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
