On 06/14/2012 01:29 PM, Thierry Reding wrote:

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

quoted

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

...

quoted

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.

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.

quoted

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.

Oh right, I missed some 8s and 0s that looked the same!

quoted

quoted

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.

But the existing driver works without /any/ devices, let alone one per
port.