Hi, On 21 August 2015 at 03:27, Thierry Reding <tred...@nvidia.com> wrote: > On Thu, Aug 20, 2015 at 11:46:41PM +0000, Marcel Ziswiler wrote: >> On 20 Aug 2015 22:09, Stephen Warren <swar...@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-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); 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
