[U-Boot] [PATCH 3/5] arm: tegra30: video: integrate display driver for t30

[Thierry Reding]

On Thu, Aug 20, 2015 at 11:46:41PM +0000, Marcel Ziswiler wrote:
> On 20 Aug 2015 22:09, Stephen Warren <swarren at wwwdotorg.org> wrote:
> 
> > Hopefully the process was to copy the Linux Tegra30 DT verbatim?
> 
> No, the T20 one is far from verbatim neither. So I just did the
> adjustments analogous by comparing the T20 and T30 Linux DTs.

I agree with Stephen that U-Boot should use an exact copy of the DTS
files in the kernel. The bits in the kernel get much more review and
have been known to work well for a number of years.

> > That's
> > far more likely to yield a correct DT than copying the Tegra20 DT to
> > Tegra30 and then patching it until it works.
> 
> I guess U-Boot has anyway about zero functionality dependency on that
> part of the device trees.

U-Boot depending on the device tree or not is really an orthogonal
issue. If we keep a delta between U-Boot and Linux DTS files, we'll risk
breaking things badly if we ever do end up putting device tree sources
into a common source tree, because we'd be exposing U-Boot to something
that it wasn't tested against.

> > If this DT doesn't exactly
> > match the Linux kernel, this needs to be fixed.
> 
> Well, then any Tegra device tree currently in U-Boot needs serious
> fixing. I usually tend to at least not make the mess any worse.

That's certainly a true statement. There really should never have been
such a disconnect as we currently have. If we can't agree on using the
very same DTS files for both U-Boot and Linux we might as well not use
device tree at all (in U-Boot).

Perhaps a good idea would be to simply copy what we have in the kernel
and see where (if at all) U-Boot breaks down and fix it to work properly
with "upstream" DTBs.

I can't obviously force you to do all that work to fix past mistakes,
but I'd like to see at least new DTS content in U-Boot deviate from what
we have upstream in the kernel.

> > > diff --git a/arch/arm/mach-tegra/tegra30/Makefile b/arch/arm/mach-tegra/tegra30/Makefile
> >
> > > -obj-$(CONFIG_SPL_BUILD) += cpu.o
> > > +ifdef CONFIG_SPL_BUILD
> > > +obj-y        += cpu.o
> >
> >
> > I don't think there's any need to edit the cpu.o line. While you can
> > move it into the ifdef like that, I don't see a need.
> 
> I can sure revert this then.
> 
> > > diff --git a/arch/arm/mach-tegra/tegra30/display.c b/arch/arm/mach-tegra/tegra30/display.c
> >
> > I didn't review this file in detail; I'll leave that to Thierry since he
> > knows the display HW.
> >
> > However, one question: Is this file a complete cut/paste of
> > tegra20/display.c, or does it just replace some parts of it? Hopefully
> > this patch doesn't simply duplicate the whole driver?
> 
> Yes, for now this is  an exact one-to-one copy but I lack the detailed
> knowledge about Tegra graphics to know whether this is much sane.

It looks to me like this adds even a third copy. There's already a
duplicate of the display controller bits in drivers/video/tegra124 along
with some new code to support eDP. There really isn't much reason for
duplicating the drivers since the display controllers haven't changed
very much over the various SoC generations. And especially in U-Boot I
don't think you'll need fancy features like color conversion or gamma
correction, or smart dimming, which are really the only areas where
there have been changes.

If you look at the Linux kernel driver we get away with very minimal
parameterization, and I think the same should be possible for U-Boot.

In a similar vein I think it should be possible to write unified drivers
for each type of output (RGB, HDMI, DSI, SOR). Those require even less
parameterization since there have been almost no changes to those since
their introduction, the rare exception being fancy features that I don't
think would be needed for U-Boot.

Granted, U-Boot doesn't give you much of a framework to work with, so
there's a lot of code that would need to be written to abstract things,
but I think that's effort well spent, much better than simply
duplicating for each new generation.

> On first sight to me the current split between hardware agnostic (e.g.
> driver/video/tegra.c) and hardware specific (e.g.
> mach-tegra/<SoC>/display.c) seems rather arbitrary. Downstream [1] I
> actually took a more radical approach and if that is rather accepted
> I'm happy to rework this patch set in that direction as well. But then
> actually completely merging tegra.c and display.c might even make more
> sense. I'm open to suggestions really.
> 
> [1] http://git.toradex.com/cgit/u-boot-toradex.git/commit/?h=2015.04-toradex-next&id=5a472ddd7a2a017747d6c05c65eba2cd3804c02f

Frankly I think it should all move into a separate "tegra" subdirectory
in drivers/video/. There's likely going to be quite a few files if we
want to support several types of outputs, and a subdirectory will help
keep things organized.

I think a framework for U-Boot could be fairly simple and doesn't have
to have the complexity of DRM/KMS in the kernel. I'd expect the U-Boot
configuration to be statically defined, and if the "framework" is Tegra
specific it doesn't need to be as generic either.

Perhaps something as simple as:

	struct tegra_dc {
		...
		int (*enable)(struct tegra_dc *dc, const struct display_mode *mode);
		void (*disable)(struct tegra_dc *dc);
		...
	};

	struct tegra_output {
		...
		struct tegra_dc *dc;
		...
		int (*enable)(struct tegra_output *output, const struct display_mode *mode);
		void (*disable)(struct tegra_output *output);
		...
	};

would work fine. That's roughly how drivers are implemented in the
kernel. Setting up display on an output would be done by determining the
mode (typically by parsing EDID if available, or using a hard-coded mode
otherwise) and then calling:

	output->dc = dc;
	dc->enable(dc, mode);
	output->enable(output, mode);

You might want to add in an abstraction for panels as well to make sure
you have enough flexibility to enable and disable those, too. In that
case you'd probably want to complement the above sequence with:

	panel->enable(panel);

Which should work for everything, except maybe DSI, where you may need
some sort of inbetween step for panels that need additional setup using
DCS commands or the like. But I suspect that's a bridge that can be
crossed when we get to it.

That said, I don't forsee myself having any time to devote to this, but
if anyone ends up spending work on this, feel free to Cc me on patches
or ask if you have questions about the display hardware or the framework
design. I'm sure I can find the time to provide feedback.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150821/fbcba2e6/attachment.sig>