On 6/12/2012 9:52 PM, Thierry Reding wrote:
On Tue, Jun 12, 2012 at 09:28:51PM -1000, Mitch Bradley wrote:
On 6/12/2012 8:45 PM, Thierry Reding wrote:
* Mitch Bradley wrote:
On 6/12/2012 10:15 AM, Stephen Warren wrote:
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";
};
reg =<1> ?
That would save having to put 2 entries in the reg, and perhaps remove
the need for any ranges property.
ISTM that having two reg entries, specifying the rp and ctrl
registers, is preferable to having code to calculate the addresses.
That makes the code simpler and the device tree more directly
descriptive of the hardware layout. The less "magic" (in this case,
the register address calculation), the better.
The problem with this approach is that since the control registers are
within the AFI register range, both the reg and ranges properties of the
parent would have to include these holes. Furthermore it means that the
controller driver would have to remap the AFI registers in chunks, for the
sole reason of splitting out the controle registers.
Would it be acceptable to make an exception in this case and use the port's
second reg entry as an offset into the AFI register range instead?
How about this:
pcie-controller {
compatible = "nvidia,tegra20-pcie";
reg =<0x80003000 0x00000800 /* PADS registers */
0x80003800 0x00000200> /* AFI registers */
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 */
I think the configuration spaces and downstream I/O ranges need to be in the
pcie-controller's reg property because they are remapped and used by the
controller driver, not by the individual ports.
That's probably not really necessary but rather a result of how the driver
was written. Perhaps the driver should handle them differently instead,
listing the regions in the ranges property of the parent and listing the
corresponding partitions in the ranges properties of the pci child nodes.
Like in the following, where the ranges property of the ports partition the
ranges passed from the parent evenly:
pcie-controller {
compatible = "nvidia,tegra20-pcie";
reg =<0x80003000 0x00000800 /* PADS registers */
0x80003800 0x00000200>; /* AFI registers */
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 0x80100000 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>;
pci@80000000 {
reg =<0x80000000 0x00001000>;
status = "disabled";
#address-cells =<3>;
#size-cells =<2>;
ranges =<0x80400000 0x80400000 0x00008000 /* I/O */
0x90000000 0x90000000 0x08000000 /* non-prefetchable memory */
0xa0000000 0xa0000000 0x08000000>; /* prefetchable memory */
You are on the right track here, but the format of the child-address
portion of the above ranges property is incorrect. Since the child
address space is the PCI address space, the child-address portion needs
to be 3 cells. It's not a linear address but rather a triple. The
first cell identifies the address type (config, I/O, memory..) and the
second and third cells are offsets within that subspace. The second and
third cells will typically be 0. The PCI binding has details.
nvidia,ctrl-offset =<0x0>;
nvidia,num-lanes =<2>;
};
pci@80001000 {
reg =<0x80001000 0x00001000>;
status = "disabled";
#address-cells =<3>;
#size-cells =<2>;
ranges =<0x80408000 0x80408000 0x00008000 /* I/O */
0x98000000 0x98000000 0x08000000 /* non-prefetchable memory */
0xa8000000 0xa8000000 0x08000000>; /* prefetchable memory */
nvidia,ctrl-offset =<0x8>;
nvidia,num-lanes =<2>;
};
};
I've also added the new num-lanes property that describes the physical
connections of the port. Also the ctrl-offset and num-lanes property are
pretty specific to the Tegra PCIe controller, so I've prefixed them with
"nvidia,".
#address-cells =<1>;
#size-cells =<1>;
pci@80000000 {
reg =<0x80000000 0x00001000>;
ctrl-offset =<0x0>;
status = "disabled";
#address-cells =<3>; /* Standard for PCI */
#size-cells =<2>; /* Standard for PCI */
ranges =</* Map from standard PCI child address spaces
to linear spaces above */>;
};
pci@80001000 {
reg =<0x80001000 0x00001000>;
ctrl-offset =<0x8>;
status = "disabled";
#address-cells =<3>; /* Standard for PCI */
#size-cells =<2>; /* Standard for PCI */
ranges =</* Map from standard PCI child address spaces
to linear spaces above */>;
};
};
Using ctrl-offset instead of reg avoids any violation of the usual
reg semantics.
Yes, that sounds better.
Note that the pci subnodes need the full complement of PCI bus node
properties.
Thierry
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
