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

[Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 5024 bytes --]

* 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...)

Yeah, I currently have code that gets the port index and the reset register
offset (ctrl_offset above) from the port address. It feels rather hackish,
though.

> 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";
> 	};
> 
> That would save having to put 2 entries in the reg, and perhaps remove
> the need for any ranges property.
> 
> I think you also need a property to specify the exact port layout; the
> Tegra20 controller supports:
> 
> 1 x4 port
> 2 x2 ports (you can choose to use only 1 of these I assume)
> 
> So just because only 1 of the ports is enabled, doesn't imply it's x4;
> it could still be x2.
> 
> Tegra30 has more options.

Both the upstream and downstream drivers currently hard-code this to dual and
411 (I assume that means 1x4, 2x1?) configurations for Tegra20 and Tegra30
respectively. Unfortunately the register AFI_PCIE_CONFIG isn't documented on
Tegra20 at all and for Tegra30 lists only the valid configurations (see
28.4.1.5 PCIe CONFIG) but not the corresponding encodings.

Maybe a good name for the new property would be "num-lanes". I also wonder if
this property would be better off in the parent node, which would make it
easier to check for valid configurations. Otherwise the code would have to
collect the settings of all the ports and check if the combination is valid.
Then again, having num-lanes in the parent with one cell for each controller
isn't very nice if each controller can be individually disabled.

One other thing: in addition to the device tree binding, this will all have to
be represented in the platform data as well to support the legacy board
definitions currently in the tree. But instead of adding all of these changes
to the patch that converts the code to a driver, I'm thinking it might be
better to split these additions off into one or more separate patches. Do you
have any objections?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]