[PATCH v2 07/10] ARM: tegra: pcie: Add device tree support

[Thierry Reding]

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 at 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 at 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 at 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?

> 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 at 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 at 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?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120621/1025714d/attachment.sig>