Re: [RFC v2 5/5] drm: Add NVIDIA Tegra support

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

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

* Stephen Warren wrote:
> On 04/25/2012 03:45 AM, Thierry Reding wrote:
> > 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.
> 
> > diff --git a/Documentation/devicetree/bindings/gpu/drm/tegra.txt b/Documentation/devicetree/bindings/gpu/drm/tegra.txt
> 
> > +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()?).

> > +	/* 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.

> > +	/* 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.

> > +		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.

> > +			#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.

> > +			connector@1 {
> > +				reg = <1>;
> > +				output = <&hdmi>;
> > +				ddc = <&i2c2>;
> > +
> > +				hpd-gpio = <&gpio 111 0>; /* PN7 */
> > +			};
> > +		};
> > +	};
> > +};
> 
> > diff --git a/arch/arm/mach-tegra/board-dt-tegra20.c b/arch/arm/mach-tegra/board-dt-tegra20.c
> 
> > +	{ "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.

> > 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

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