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

[Mitch Bradley <wmb@firmworks.com>]

On 6/12/2012 9:52 PM, Thierry Reding wrote:
> On Tue, Jun 12, 2012 at 09:28:51PM -1000, Mitch Bradley wrote:
>> On 6/12/2012 8:45 PM, Thierry Reding wrote:
>>> * Mitch Bradley wrote:
>>>> On 6/12/2012 10:15 AM, Stephen Warren wrote:
>>>>> On 06/12/2012 11:20 AM, Thierry Reding wrote:
>>>>> ...
>>>>>> I came up with the following alternative:
>>>>>>
>>>>>> 	pci {
>>>>>> 		compatible = "nvidia,tegra20-pcie";
>>>>>> 		reg =<0x80003000 0x00000800   /* PADS registers */
>>>>>> 		       0x80003800 0x00000200   /* AFI registers */
>>>>>> 		       0x80004000 0x00100000   /* configuration space */
>>>>>> 		       0x80104000 0x00100000   /* extended configuration space */
>>>>>> 		       0x80400000 0x00010000   /* downstream I/O */
>>>>>> 		       0x90000000 0x10000000   /* non-prefetchable memory */
>>>>>> 		       0xa0000000 0x10000000>; /* prefetchable memory */
>>>>>> 		interrupts =<0 98 0x04   /* controller interrupt */
>>>>>> 		              0 99 0x04>; /* MSI interrupt */
>>>>>> 		status = "disabled";
>>>>>>
>>>>>> 		ranges =<0x80000000 0x80000000 0x00002000   /* 2 root ports */
>>>>>> 			  0x80004000 0x80004000 0x00100000   /* configuration space */
>>>>>> 			  0x80104000 0x80104000 0x00100000   /* extended configuration space */
>>>>>> 			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
>>>>>> 			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
>>>>>> 			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */
>>>>>>
>>>>>> 		#address-cells =<1>;
>>>>>> 		#size-cells =<1>;
>>>>>>
>>>>>> 		port@80000000 {
>>>>>> 			reg =<0x80000000 0x00001000>;
>>>>>> 			status = "disabled";
>>>>>> 		};
>>>>>>
>>>>>> 		port@80001000 {
>>>>>> 			reg =<0x80001000 0x00001000>;
>>>>>> 			status = "disabled";
>>>>>> 		};
>>>>>> 	};
>>>>>>
>>>>>> The "ranges" property can probably be cleaned up a bit, but the most
>>>>>> interesting part is the port@ children, which can simply be enabled in board
>>>>>> DTS files by setting the status property to "okay". I find that somewhat more
>>>>>> intuitive to the variant with an "enable-ports" property.
>>>>>>
>>>>>> What do you think of this?
>>>>>
>>>>> As a general concept, this kind of design seems OK to me.
>>>>>
>>>>> The "port" child nodes I think should be named "pci@..." given Mitch's
>>>>> comments, I think.
>>>>>
>>>>> The port nodes probably need two entries in reg, given the following in
>>>>> our downstream driver:
>>>>>
>>>>>>          int rp_offset = 0;
>>>>>>          int ctrl_offset = AFI_PEX0_CTRL;
>>>>> ...
>>>>>>          for (port = 0; port<    MAX_PCIE_SUPPORTED_PORTS; port++) {
>>>>>>                  ctrl_offset += (port * 8);
>>>>>>                  rp_offset = (rp_offset + 0x1000) * port;
>>>>>>                  if (tegra_pcie.plat_data->port_status[port])
>>>>>>                          tegra_pcie_add_port(port, rp_offset, ctrl_offset);
>>>>>>          }
>>>>>
>>>>> (which actually looks likely to be horribly buggy for port>1 and only
>>>>> accidentally correct for port==1, but anyway...)
>>>>>
>>>>> But instead, I'd be tempted to make the top-level node say:
>>>>>
>>>>> 	#address-cells =<1>;
>>>>> 	#size-cells =<0>;
>>>>>
>>>>> ... so that the child nodes' reg is just the port ID. The parent node
>>>>> can calculate the addresses/offsets of the per-port registers within the
>>>>> PCIe controller's register space based on the ID using code roughly like
>>>>> what I quoted above:
>>>>>
>>>>> 	pci@0 {
>>>>> 		reg =<0>;
>>>>> 		status = "disabled";
>>>>> 	};
>>>>>
>>>>> 	pci@1 {
>>>>> 		reg =<0>;
>>>>> 		status = "disabled";
>>>>> 	};
>>>> reg =<1>   ?
>>>>
>>>>>
>>>>>
>>>>> That would save having to put 2 entries in the reg, and perhaps remove
>>>>> the need for any ranges property.
>>>>
>>>> ISTM that having two reg entries, specifying the rp and ctrl
>>>> registers, is preferable to having code to calculate the addresses.
>>>> That makes the code simpler and the device tree more directly
>>>> descriptive of the hardware layout.  The less "magic" (in this case,
>>>> the register address calculation), the better.
>>>
>>> The problem with this approach is that since the control registers are
>>> within the AFI register range, both the reg and ranges properties of the
>>> parent would have to include these holes. Furthermore it means that the
>>> controller driver would have to remap the AFI registers in chunks, for the
>>> sole reason of splitting out the controle registers.
>>>
>>> Would it be acceptable to make an exception in this case and use the port's
>>> second reg entry as an offset into the AFI register range instead?
>>
>> How about this:
>>
>>      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 =<0x80000000 0x80000000 0x00002000   /* 2 root ports */
>>                0x80004000 0x80004000 0x00100000   /* configuration space */
>>                0x80104000 0x80104000 0x00100000   /* extended
>> configuration space */
>>                0x80400000 0x80400000 0x00010000   /* downstream I/O */
>>                0x90000000 0x90000000 0x10000000   /* non-prefetchable
>> memory */
>>                0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */
>
> I think the configuration spaces and downstream I/O ranges need to be in the
> pcie-controller's reg property because they are remapped and used by the
> controller driver, not by the individual ports.
>
> That's probably not really necessary but rather a result of how the driver
> was written. Perhaps the driver should handle them differently instead,
> listing the regions in the ranges property of the parent and listing the
> corresponding partitions in the ranges properties of the pci child nodes.
>
> Like in the following, where the ranges property of the ports partition the
> ranges passed from the parent evenly:
>
> 	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 =<0x80000000 0x80000000 0x00002000   /* 2 root ports */
> 			  0x80004000 0x80004000 0x00100000   /* configuration space */
> 			  0x80104000 0x80100000 0x00100000   /* extended configuration space */
> 			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
> 			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
> 			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */
>
> 		#address-cells =<1>;
> 		#size-cells =<1>;
>
> 		pci@80000000 {
> 			reg =<0x80000000 0x00001000>;
> 			status = "disabled";
>
> 			#address-cells =<3>;
> 			#size-cells =<2>;
>
> 			ranges =<0x80400000 0x80400000 0x00008000   /* I/O */
> 				  0x90000000 0x90000000 0x08000000   /* non-prefetchable memory */
> 				  0xa0000000 0xa0000000 0x08000000>; /* prefetchable memory */

You are on the right track here, but the format of the child-address 
portion of the above ranges property is incorrect.  Since the child 
address space is the PCI address space, the child-address portion needs 
to be 3 cells.  It's not a linear address but rather a triple.  The 
first cell identifies the address type (config, I/O, memory..) and the 
second and third cells are offsets within that subspace.  The second and 
third cells will typically be 0.  The PCI binding has details.

>
>
> 			nvidia,ctrl-offset =<0x0>;
> 			nvidia,num-lanes =<2>;
> 		};
>
> 		pci@80001000 {
> 			reg =<0x80001000 0x00001000>;
> 			status = "disabled";
>
> 			#address-cells =<3>;
> 			#size-cells =<2>;
>
> 			ranges =<0x80408000 0x80408000 0x00008000   /* I/O */
> 				  0x98000000 0x98000000 0x08000000   /* non-prefetchable memory */
> 				  0xa8000000 0xa8000000 0x08000000>; /* prefetchable memory */
>
> 			nvidia,ctrl-offset =<0x8>;
> 			nvidia,num-lanes =<2>;
> 		};
> 	};
>
> I've also added the new num-lanes property that describes the physical
> connections of the port. Also the ctrl-offset and num-lanes property are
> pretty specific to the Tegra PCIe controller, so I've prefixed them with
> "nvidia,".
>
>>
>>          #address-cells =<1>;
>>          #size-cells =<1>;
>>
>>          pci@80000000 {
>>              reg =<0x80000000 0x00001000>;
>>              ctrl-offset =<0x0>;
>>              status = "disabled";
>>              #address-cells =<3>;  /* Standard for PCI */
>>              #size-cells =<2>;      /* Standard for PCI */
>>              ranges =</* Map from standard PCI child address spaces
>> to linear spaces above */>;
>>          };
>>
>>          pci@80001000 {
>>              reg =<0x80001000 0x00001000>;
>>              ctrl-offset =<0x8>;
>>              status = "disabled";
>>              #address-cells =<3>;  /* Standard for PCI */
>>              #size-cells =<2>;      /* Standard for PCI */
>>              ranges =</* Map from standard PCI child address spaces
>> to linear spaces above */>;
>>          };
>>      };
>>
>> Using ctrl-offset instead of reg avoids any violation of the usual
>> reg semantics.
>
> Yes, that sounds better.
>
>>
>> Note that the pci subnodes need the full complement of PCI bus node
>> properties.
>>
>>>
>>>
>>> Thierry
>>