* 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:
pgpDOO5V58Ftx.pgp
Description: PGP signature
