* Stephen Warren wrote:

On 04/25/2012 03:45 AM, Thierry Reding wrote:

quoted

This commit adds a very basic DRM driver for NVIDIA Tegra SoCs. It
currently has rudimentary GEM support and can run a console on the
framebuffer as well as X using the xf86-video-modesetting driver. Only
the RGB output is supported.

HDMI support was taken from NVIDIA's Linux kernel tree but it doesn't
quite work. EDID data can be retrieved but the output doesn't properly
activate the connected TV.

The DSI and TVO outputs and the HOST1X driver are just stubs that setup
the corresponding resources but don't do anything useful yet.

I'm mainly going to comment on the device tree bindings here; hopefully
Jon and Terje can chime in on the code itself.

quoted

diff --git a/Documentation/devicetree/bindings/gpu/drm/tegra.txt b/Documentation/devicetree/bindings/gpu/drm/tegra.txt

quoted

+Example:
+
+/memreserve/ 0x0e000000 0x02000000;
+
+...
+
+/ {
+	...
+
+	/* host1x */
+	host1x: host1x@50000000 {
+		compatible = "nvidia,tegra20-host1x";
+		reg = <0x50000000 0x00024000>;
+		interrupts = <0 64 0x04   /* cop syncpt */
+			      0 65 0x04   /* mpcore syncpt */
+			      0 66 0x04   /* cop general */
+			      0 67 0x04>; /* mpcore general */
+	};

The host1x module is apparently a register bus, behind which all the
other modules sit. According to the address map in the TRM, "all the
other modules" appears to include all of MPE, VI, CSI, EPP, ISP, GR2D,
GR3D, DISPLAY A/B, HDMI, TVO, DSI, plus VG, VS, VCI, DSIB on Tegra30.

That said, I believe Terje wasn't convinced that all those modules are
really host1x children, just some. Can you check please, Terje?

Also, I wonder if host1x is really the parent of these modules,
register-bus-access-wise, or whether the bus covers the "graphic host
registers" at 0x50000000-0x50023fff?

As such, I think the DT nodes for all those modules should be within the
host1x node (or perhaps graphics host node, pending above discussion):

    host1x: host1x@50000000 {
        mpe@54040000 { ... };
        vi@54080000 { ... };
        ...
    };

host1x can easily instantiate all the child nodes simply by calling
of_platform_populate() at the end of its probe; see
sound/soc/tegra/tegra30_ahub.c for an example.

I actually had an implementation at some point that did exactly that. The
problem was that there is no equivalent to of_platform_populate() to call at
module removal, so that when the tegra-drm module was unloaded and reloaded,
it would try to create duplicate devices.

Something ugly can probably be done with device_for_each_child(), but maybe a
better option would be to add a helper to the DT code to explicitly undo the
effects of of_platform_populate() (of_platform_desolate()?).

quoted

+	/* video-encoding/decoding */
+	mpe@54040000 {
+		reg = <0x54040000 0x00040000>;
+		interrupts = <0 68 0x04>;
+	};

We'll probably end up needing a phandle from each of these nodes to
host1x, and a channel ID, so the drivers for these nodes can register
themselves with host1x. However, I it's probably OK to defer the DT
binding for this until we actually start working on command-channels.

I suppose these should now also drop the unit addresses to follow your
cleanup patches of the DTS files.

quoted

+	/* graphics host */
+	graphics@54000000 {
+		compatible = "nvidia,tegra20-graphics";
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;

I don't think those 3 properties are needed, unless there are child
nodes with registers.

Right, those are relics from the earlier version I mentioned above, where the
other nodes were children of the graphics node.

quoted

+		display-controllers = <&disp1 &disp2>;
+		carveout = <0x0e000000 0x02000000>;
+		host1x = <&host1x>;
+		gart = <&gart>;
+
+		connectors {

I'm not sure that it makes sense to put the connectors nodes underneath
the graphics host node; the connectors aren't objects with registers
that are accessed through a bus managed by the graphics node. Equally,
the connectors could be useful outside of the graphics driver - e.g. the
audio driver might need to refer to an HDMI connector.

Instead, I'd probably put the connector nodes at the top level of the
device tree, and have graphics contain a property that lists the
phandles of all available connectors.

That makes a lot of sense.

quoted

+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			connector@0 {
+				reg = <0>;
+				edid = /incbin/("machine.edid");
+				output = <&lvds>;
+			};

I think each of these needs some kind of compatible value to indicate
their type. Also, the ability to represent both HDMI video and audio
streams so that a sound card binding could use the HDMI connector too. I
see that drivers/extcon/ has appeared recently, and I wonder if the
connector nodes in DT shouldn't be handled by that subsystem?

I just took a brief look at it. I'll have to play around with it a bit to see
how it fits.

quoted

+			connector@1 {
+				reg = <1>;
+				output = <&hdmi>;
+				ddc = <&i2c2>;
+
+				hpd-gpio = <&gpio 111 0>; /* PN7 */
+			};
+		};
+	};
+};

quoted

diff --git a/arch/arm/mach-tegra/board-dt-tegra20.c b/arch/arm/mach-tegra/board-dt-tegra20.c

quoted

+	{ "host1x",     "pll_c",        144000000,      true },
+	{ "disp1",      "pll_p",        600000000,      true },
+	{ "disp2",      "pll_p",        600000000,      true },

Can we use these tables /just/ to set up the clock parent relationships,
and rely on the drivers to enable/disable the clocks as needed? I'm
hoping to replace these tables with DT once Tegra support common clock
and bindings, but I don't want to see the ability to turn clocks on
there, just set the parenting relationships, and perhaps initial rates.

I did try setting the enabled field to false, but that broke RGB output. I
didn't have much time to investigate why. Once Tegra moves to the common
clock bindings this should definitely be fixed properly.

quoted

diff --git a/arch/arm/mach-tegra/include/mach/iomap.h b/arch/arm/mach-tegra/include/mach/iomap.h
index 7e76da7..3e80f3f 100644
--- a/arch/arm/mach-tegra/include/mach/iomap.h
+++ b/arch/arm/mach-tegra/include/mach/iomap.h

@@ -56,6 +56,12 @@
 #define TEGRA_HDMI_BASE			0x54280000
 #define TEGRA_HDMI_SIZE			SZ_256K
 
+#define TEGRA_TVO_BASE			0x542c0000
+#define TEGRA_TVO_SIZE			SZ_256K
+
+#define TEGRA_DSI_BASE			0x54300000
+#define TEGRA_DSI_SIZE			SZ_256K

Perhaps we can just hard-code the addresses into the AUXDATA in
board-dt-tegra20.c to save defining more entries in this file. Hopefully
this file is going away ASAP when we get rid of board files.

Okay, I can do that.

Thierry