On Thu, Jun 14, 2012 at 12:30:50PM -0600, Stephen Warren wrote:

On 06/14/2012 03:19 AM, Thierry Reding wrote:
...

quoted

Okay, so the new pcie-controller node looks like this:

pcie-controller { compatible = "nvidia,tegra20-pcie",
"simple-bus"; 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 */

Do we actually need to specifically enumerate all the ranges; would
just "ranges;" be simpler?

I think they need to be listed here in order for the child nodes to be able
to map them using their ranges property. It's possible that it'll work by
specifying an empty ranges, but I don't think it'd be correct in the OF
sense.

quoted

#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.

quoted

compatible = "nvidia,tegra20-pcie-port"; reg = <0x80000000
0x00001000>; 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
*/

The values here appear identical for both ports. Surely they should
describe just the parts of the overall address space that have been
assigned/delegated to the individual port/bridge?

They're not identical. Port 0 gets the first half and port 1 gets the second
half of the ranges specified in the parent.

Note that initial indications are that the overall controller receives
transactions for those address ranges, and standard PCIe bridge
registers are used to steer chunks of these address ranges into the
individual downstream ports/busses. Hence, standard PCIe documentation
should indicate how to do this.

Yes, I believe that works with the code I have currently, which basically
parses the ranges property of each port and initializes the I/O, memory and
prefetchable regions from those values.

quoted

nvidia,ctrl-offset = <0x110>; nvidia,num-lanes = <2>; };

pci@80001000 { compatible = "nvidia,tegra20-pcie-port"; reg =
<0x80001000 0x00001000>; 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>; }; };

While looking into some more code, trying to figure out how to hook
this all up with the device tree I ran into a problem. I need to
actually create a 'struct device' for each of the ports, so I added
the "simple-bus" to the pcie-controller's "compatible" property.
Furthermore, each PCI root port now becomes a platform_device,
which are supported by a new tegra-pcie-port driver. I'm not sure
if "port" is very common in PCI speek, so something like 
tegra-pcie-bridge (compatible = "nvidia,tegra20-pcie-bridge") may
be more appropriate?

What is it that drives the need for each port to be a 'struct device'?
The current driver supports 2 host ports, yet there's only a single
struct device for it. Does the DT code assume a 1:1 mapping between
struct device and DT node that represents the child bus? If so,
perhaps it'd be better to rework that code to accept a DT node as a
parameter and call it multiple times, rather than accept a struct
device as a parameter and hence need multiple devices?

It's not so much the DT code, but rather the PCI core and ultimately the
device model that requires it. Each port is basically a PCI host bridge that
provides a root PCI bus and the device model is used to represent the
hierachy of the busses. Providing just the DT node isn't going to be enough.

Thierry