Hi,
On 21 August 2015 at 03:27, Thierry Reding treding@nvidia.com wrote:
On Thu, Aug 20, 2015 at 11:46:41PM +0000, Marcel Ziswiler wrote:
On 20 Aug 2015 22:09, Stephen Warren swarren@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.
I have serious doubts about the wisdom of requiring a contributor to completely re-architect the existing display system in U-Boot. It's a big job. Perhaps we can settle for following along the same lines and not making things worse?
That said, if Marcel has the time, I may as well pile on with a few more comments :-)
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-nex...
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);
Please don't add function points to structures on an ad-hoc basis. These should use driver model. There is a uclass for display port but not for LCD panels or SOR. You could add a very simple one for a panel if you like. Please take a look at tegra124's display driver for an example.
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.
In which case I suggest we limit the amount of rewrite we ask for in this case...
Regards, Simon