Re: [RFC 4/4] drm: Add NVIDIA Tegra support

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 04/13/2012 03:14 AM, Thierry Reding wrote:
> * Stephen Warren wrote:
>> On 04/12/2012 11:44 AM, Thierry Reding wrote:
> [...]
>> And given that, I don't think we should name the node after some
>> OS-specific software concept. Device tree is intended to model hardware.
> [...]
>>> Maybe one solution would be to have a top-level DRM device with a register
>>> map from 0x54000000 to 0x547fffff, which the TRM designates as "host
>>> registers". Then subnodes could be used for the subdevices.
>>
>> Ah yes, just what I was thinking above:-)
> 
> I came up with the following:
> 
> 	/* host1x */
> 	host1x : host1x@50000000 {
> 		reg = <0x50000000 0x00024000>;
> 		interrupts = <0 64 0x04   /* cop syncpt */
> 			      0 65 0x04   /* mpcore syncpt */
> 			      0 66 0x04   /* cop general */
> 			      0 67 0x04>; /* mpcore general */
> 	};
> 
> 	/* graphics host */
> 	graphics@54000000 {
> 		compatible = "nvidia,tegra20-graphics";
> 
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 		ranges = <0 0x54000000 0x08000000>;
> 
> 		host1x = <&host1x>;
> 
> 		/* video-encoding/decoding */
> 		mpe@54040000 {
> 			reg = <0x54040000 0x00040000>;
> 			interrupts = <0 68 0x04>;
> 		};
... [a bunch of nodes for graphics-related HW modules]

That all looks reasonable.

> 		display-controllers = <&disp1 &disp2>;
> 		outputs = <&lvds &hdmi &tvo &dsi>;

I don't think you need both the child nodes and those two properties.

In other words, I think you either want:

	graphics@54000000 {
		... a bunch of child nodes
	};

or you want:

	disp1 : dc@54200000 {
		...
	};
	disp2 : dc@54240000 {
		...
	};
	... all the other graphics nodes

	graphics@54000000 {
		display-controllers = <&disp1 &disp2>;
		outputs = <&lvds &hdmi &tvo &dsi>;
	};

In the former case, presumably the drivers for the child nodes would
make some API call into the parent node and just register themselves
directly as a certain type of driver, so avoiding the
display-controllers/outputs properties.

> 		/* initial configuration */
> 		configuration {
> 			lvds {
> 				display-controller = <&disp1>;
> 				output = <&lvds>;
> 			};
> 
> 			hdmi {
> 				display-controller = <&disp2>;
> 				output = <&hdmi>;
> 			};
> 		};
> 	};
> 
> I added an additional node for the initial configuration so that the driver
> knows which mapping to setup at boot.

Isn't that kind of thing usually set up by the video= KMS-related kernel
command-line option? See Documentation/fb/modedb.txt. Again here, I
think the actual display controllers would be allocated to whichever
outputs get used on a first-come first-serve based, so no need for the
display-controller property above either way.

> What I don't quite see yet is where to
> attach EDID data or pass the phandle to the I2C controller for DDC/EDID
> probing.

I think there's a third node type.

1) Nodes for the display controllers in Tegra (Also known as CRTCs I
believe)

2) Nodes for the video outputs from the Tegra chips (what you have as
outputs above)

3) Connectors, which aren't represented in your DT above yet.

I think you need to add additional nodes for (3). These are where you'd
represent all the EDID/I2C/... stuff. Then, the nodes for the outputs
will point at the nodes for the connectors using phandles, or vice-versa.

(IIRC, although I didn't look at them in detail, the sdrm patches
recently mentioned something about splitting up the various object types
in DRM and allowing them to be represented separately, and I vaguely
recall something along the lines of the types I mention above. I might
expect to have a 1:1 mapping between the DRM object types and DT nodes.)

> The initial configuration is certainly not the right place. Perhaps
> the outputs property should be made a node instead:
> 
> 	outputs {
> 		lvds_out {
> 			output = <&lvds>;
> 			edid = <&edid>;
> 		};
> 
> 		hdmi_out {
> 			output = <&hdmi>;
> 			ddc = <&i2c2>;
> 		};
> 	};
> 
> But then "outputs" should probably become something like "connectors"
> instead and the initial configuration refers to the "_out" phandles.