[PATCH v2 07/10] ARM: tegra: pcie: Add device tree support
Mitch Bradley
wmb at firmworks.com
Tue Jun 19 17:31:39 EDT 2012
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.
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>;
};
Version B - 2 address cells: In this first version, the intermediate
address space has 2 cells: port#, offset. The address type (I/O, mem,
etc) is the high digit of in the offset:
Offset 0....... : Root port
Offset 1....... : config
Offset 2....... : extended config
Offset 3....... : I/O
Offset 4....... : non-prefetchable memory
Offset 5....... : prefetchable memory.
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 = <0 0x00000000 0x80000000 0x00001000 /* Root port 0 */
0 0x10000000 0x80004000 0x00080000 /* Port 0 config */
0 0x20000000 0x80104000 0x00080000 /* Port 0 ext config */
0 0x30000000 0x80400000 0x00008000 /* Port 0 downstream I/O */
0 0x40000000 0x90000000 0x08000000 /* Port 0 non-prefetchable memory */
0 0x50000000 0xa0000000 0x08000000 /* Port 0 prefetchable memory */
1 0x00000000 0x80001000 0x00001000 /* Root port 1 */
1 0x10000000 0x80084000 0x00080000 /* Port 1 config */
1 0x20000000 0x80184000 0x00080000 /* Port 1 ext config */
1 0x30000000 0x80408000 0x00010000 /* Port 1 downstream I/O */
1 0x40000000 0x98000000 0x08000000 /* Port 1 non-prefetchable memory */
1 0x50000000 0xa0000000 0x08000000>; /* Port 1 prefetchable memory */
#address-cells = <2>;
#size-cells = <1>;
pci at 0 {
reg = <0 0 0x1000>;
status = "disabled";
#address-cells = <3>;
#size-cells = <2>;
ranges = <0x80000000 0 0 0 0x10000000 0 0x00080000 /* config */
0x90000000 0 0 0 0x20000000 0 0x00080000 /* ext config */
0x81000000 0 0 0 0x30000000 0 0x00008000 /* I/O */
0x82000000 0 0 0 0x40000000 0 0x08000000 /* non-prefetchable memory */
0xc2000000 0 0 0 0x50000000 0 0x08000000>; /* prefetchable memory */
nvidia,ctrl-offset = <0x110>;
nvidia,num-lanes = <2>;
};
pci at 1 {
reg = <1 0 0x1000>;
status = "disabled";
#address-cells = <3>;
#size-cells = <2>;
ranges = <0x80000000 0 0 1 0x10000000 0 0x00080000 /* config */
0x90000000 0 0 1 0x20000000 0 0x00080000 /* ext config */
0x81000000 0 0 1 0x30000000 0 0x00008000 /* I/O */
0x82000000 0 0 1 0x40000000 0 0x08000000 /* non-prefetchable memory */
0xc2000000 0 0 1 0x50000000 0 0x08000000>; /* prefetchable memory */
nvidia,ctrl-offset = <0x118>;
nvidia,num-lanes = <2>;
};
Version C - 2 address cells, 0 size cells (!) : In this version we hide the size component in the intermediate space. I don't know if that will actually work, but my guess is that it probably would. The intermediate address space is like version A with the omission of the offset field. The intermediate address just specifies the port number and the address type.
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 = <0 0 0x80000000 /* Root port 0 */
0 1 0x80004000 /* Port 0 config */
0 2 0x80104000 /* Port 0 ext config */
0 3 0x80400000 /* Port 0 downstream I/O */
0 4 0x90000000 /* Port 0 non-prefetchable memory */
0 5 0xa0000000 /* Port 0 prefetchable memory */
1 0 0x80001000 /* Root port 1 */
1 1 0x80084000 /* Port 1 config */
1 2 0x80184000 /* Port 1 ext config */
1 3 0x80408000 /* Port 1 downstream I/O */
1 4 0x98000000 /* Port 1 non-prefetchable memory */
1 5 0xa0000000>; /* Port 1 prefetchable memory */
#address-cells = <2>;
#size-cells = <0>;
pci at 0 {
reg = <0 0>;
status = "disabled";
#address-cells = <3>;
#size-cells = <2>;
ranges = <0x80000000 0 0 0 1 0 0x00080000 /* config */
0x90000000 0 0 0 2 0 0x00080000 /* ext config */
0x81000000 0 0 0 3 0 0x00008000 /* I/O */
0x82000000 0 0 0 4 0 0x08000000 /* non-prefetchable memory */
0xc2000000 0 0 0 5 0 0x08000000>; /* prefetchable memory */
nvidia,ctrl-offset = <0x110>;
nvidia,num-lanes = <2>;
};
pci at 1 {
reg = <1 0>;
status = "disabled";
#address-cells = <3>;
#size-cells = <2>;
ranges = <0x80000000 0 0 1 1 0 0x00080000 /* config */
0x90000000 0 0 1 2 0 0x00080000 /* ext config */
0x81000000 0 0 1 3 0 0x00008000 /* I/O */
0x82000000 0 0 1 4 0 0x08000000 /* non-prefetchable memory */
0xc2000000 0 0 1 5 0 0x08000000>; /* prefetchable memory */
nvidia,ctrl-offset = <0x118>;
nvidia,num-lanes = <2>;
};
In version C, you might even be able to include the ctrl register offset as a second entry in the reg property.
More information about the linux-arm-kernel
mailing list