On 6/20/2012 8:47 PM, Thierry Reding wrote:
On Tue, Jun 19, 2012 at 11:31:39AM -1000, Mitch Bradley wrote:
On 6/19/2012 3:30 AM, Thierry Reding wrote:
On Fri, Jun 15, 2012 at 08:12:36AM +0200, Thierry Reding wrote:
On Thu, Jun 14, 2012 at 01:50:56PM -0600, Stephen Warren wrote:
On 06/14/2012 01:29 PM, Thierry Reding wrote:
On Thu, Jun 14, 2012 at 12:30:50PM -0600, Stephen Warren wrote:
On 06/14/2012 03:19 AM, Thierry Reding wrote:
...
#address-cells = <1>; #size-cells = <1>;
pci@80000000 {
I'm still not convinced that using the address of the port's
registers is the correct way to represent each port. The port
index seems much more useful.
The main reason here is that there are a lot of registers that
contain fields for each port - far more than the combination of
this node's reg and ctrl-offset (which I assume is an address
offset for just one example of this issue) properties can
describe. The bit position and bit stride of these fields isn't
necessarily the same in each register. Do we want a property like
ctrl-offset for every single type of field in every single shared
register that describes the location of the relevant data, or
just a single "port ID" bit that can be applied to anything?
(Perhaps this isn't so obvious looking at the TRM since it
doesn't document all registers, and I'm also looking at the
Tegra30 documentation too, which might be more exposed to this -
I haven't correlated all the documentation sources to be sure
though)
I agree that maybe adding properties for each bit position or
register offset may not work out too well. But I think it still
makes sense to use the base address of the port's registers (see
below). We could of course add some code to determine the index
>from the base address at initialization time and reuse the index
where appropriate.
To me, working back from address to ID then using the ID to calculate
some other addresses seems far more icky than just calculating all the
addresses based off of one ID. But, I suppose this doesn't make a huge
practical difference.
This really depends on the device vs. no device decision below. If we can
make it work without needing an extra device for it, then using the index
is certainly better. However, if we instantiate devices from the DT, then
we have the address anyway and adding the index as a property would be
redundant and error prone (what happens if somebody sets the index of the
port at address 0x80000000 to 2?).
An additional problem with this is that we'd have to add the following
to the pcie-controller node:
#address-cells = <1>;
#size-cells = <0>;
This will conflict with the "ranges" property, because suddenly we can
no longer map the regions properly. Maybe Mitch can comment on whether
this is possible or not?
To make it clearer what I'm talking about, here's the DT snippet again
(with the compatible property removed from the pci@ nodes because they
are no longer probed by a driver, the "simple-bus" removed from the
pcie-controller node's compatible property removed and its #address-
and #size-cells properties adjusted as described above).
pcie-controller {
compatible = "nvidia,tegra20-pcie";
reg = <0x80003000 0x00000800 /* PADS registers */
0x80003800 0x00000200 /* AFI registers */
0x80004000 0x00100000 /* configuration space */
0x80104000 0x00100000>; /* extended configuration space */
interrupts = <0 98 0x04 /* controller interrupt */
0 99 0x04>; /* MSI interrupt */
status = "disabled";
ranges = <0x80000000 0x80000000 0x00002000 /* 2 root ports */
0x80400000 0x80400000 0x00010000 /* downstream I/O */
0x90000000 0x90000000 0x10000000 /* non-prefetchable memory */
0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */
#address-cells = <1>;
#size-cells = <0>;
pci@0 {
reg = <2>;
status = "disabled";
#address-cells = <3>;
#size-cells = <2>;
ranges = <0x81000000 0 0 0x80400000 0 0x00008000 /* I/O */
0x82000000 0 0 0x90000000 0 0x08000000 /* non-prefetchable memory */
0xc2000000 0 0 0xa0000000 0 0x08000000>; /* prefetchable memory */
nvidia,ctrl-offset = <0x110>;
nvidia,num-lanes = <2>;
};
pci@1 {
reg = <1>;
status = "disabled";
#address-cells = <3>;
#size-cells = <2>;
ranges = <0x81000000 0 0 0x80408000 0 0x00008000 /* I/O */
0x82000000 0 0 0x98000000 0 0x08000000 /* non-prefetchable memory */
0xc2000000 0 0 0xa8000000 0 0x08000000>; /* prefetchable memory */
nvidia,ctrl-offset = <0x118>;
nvidia,num-lanes = <2>;
};
};
AIUI none of the ranges properties are valid anymore, because the bus
represented by pcie-controller no longer reflects the truth, namely that
it translates the CPU address space to the PCI address space.
I think you can use a small-integer port number as an address by
defining the intermediate address space properly, and using
appropriate ranges above and below. Here's a swag at how that would
look.
I present three versions, using different choices for the
intermediate address space encoding. The intermediate address space
may seem somewhat artificial, in that it decouples the "linear pass
through of ranges" between the lower PCI address space and the upper
CPU address space. But in another sense, it accurately reflects
that fact that the bus bridge "slices and dices" that linear address
space into non-contiguous pieces.
Note that I'm also fixing a problem that I neglected to mention
earlier - namely the fact that config space is part of the child PCI
address space so it must be passed through.
This doesn't quite match how the Tegra PCIe controller works. Basically,
every access to a device's (extended) configuration space goes through a
single region of memory-mapped registers, independent of which port is
the parent of the device.
Is it okay to pass both configuration spaces via the ranges property but
still list them in the reg property of the controller?
It's a small hierarchy violation, but I expect that it would work in
practice.
Version A - 3 address cells: In this version, the intermediate
address space has 3 cells: port#, address type, offset. Address
type is
0 : root port
1 : config space
2 : extended config space
3 : I/O
4 : non-prefetchable memory
5 : prefetchable memory.
The third cell "offset" is necessary so that the size field has a
number space that can include it.
pcie-controller {
compatible = "nvidia,tegra20-pcie";
reg = <0x80003000 0x00000800 /* PADS registers */
0x80003800 0x00000200>; /* extended configuration space */
interrupts = <0 98 0x04 /* controller interrupt */
0 99 0x04>; /* MSI interrupt */
status = "disabled";
ranges = <0 0 0 0x80000000 0x00001000 /* Root port 0 */
0 1 0 0x80004000 0x00080000 /* Port 0 config space */
0 2 0 0x80104000 0x00080000 /* Port 0 ext config space *
0 3 0 0x80400000 0x00008000 /* Port 0 downstream I/O */
0 4 0 0x90000000 0x08000000 /* Port 0 non-prefetchable memory */
0 5 0 0xa0000000 0x08000000 /* Port 0 prefetchable memory */
1 0 0 0x80001000 0x00001000 /* Root port 1 */
1 1 0 0x80004000 0x00080000 /* Port 1 config space */
1 2 0 0x80184000 0x00080000 /* Port 1 ext config space */
1 3 0 0x80408000 0x00010000 /* Port 1 downstream I/O */
1 4 0 0x98000000 0x08000000 /* Port 1 non-prefetchable memory */
1 5 0 0xa0000000 0x08000000>; /* Port 1 prefetchable memory */
#address-cells = <3>;
#size-cells = <1>;
pci@0 {
reg = <0 0 0 0x1000>;
status = "disabled";
#address-cells = <3>;
#size-cells = <2>;
ranges = <0x80000000 0 0 0 1 0 0 0x00080000 /* config */
0x90000000 0 0 0 2 0 0 0x00080000 /* extended config */
0x81000000 0 0 0 3 0 0 0x00008000 /* I/O */
0x82000000 0 0 0 4 0 0 0x08000000 /* non-prefetchable memory */
0xc2000000 0 0 0 5 0 0 0x08000000>; /* prefetchable memory */
nvidia,ctrl-offset = <0x110>;
nvidia,num-lanes = <2>;
};
pci@1 {
reg = <1 0 0 0x1000>;
status = "disabled";
#address-cells = <3>;
#size-cells = <2>;
ranges = <0x80000000 0 0 1 1 0 0 0x00080000 /* config */
0x90000000 0 0 1 2 0 0 0x00080000 /* extended config */
0x81000000 0 0 1 3 0 0 0x00008000 /* I/O */
0x82000000 0 0 1 4 0 0 0x08000000 /* non-prefetchable memory */
0xc2000000 0 0 1 5 0 0 0x08000000>; /* prefetchable memory */
nvidia,ctrl-offset = <0x118>;
nvidia,num-lanes = <2>;
};
Everybody seems to be happy with this approach, so I'll give it a shot.
There is one thing I'm still unsure about, though. What if somebody uses
the above scheme and maps the registers to the wrong port. The same goes
for the nvidia,ctrl-offset property. It needs to match the register
offset because they are directly related. I suppose we could leave that
property away and look up the register via the port index (which, as
Stephen already said, we'll have to do in other places anyway, unless we
list all bit positions in the DT).
Can we safely ignore such issues and assume the device tree to always be
right? Should we just not care if somebody uses it wrongly?
To the extent that the device tree is the definitive answer about the
system addressing, if you get it wrong, either the system will not work
or the OS isn't paying attention to that part of the device tree,
hardcoding the right answer.
Thierry
--
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
