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

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

[Marcel Ziswiler]

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.

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

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

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

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

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

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

[Simon Glass]

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-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] [PATCH 3/5] arm: tegra30: video: integrate display driver for t30

[Thierry Reding]

On Fri, Aug 21, 2015 at 06:37:37PM -0600, Simon Glass wrote:
[...]
> 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?

I didn't suggest re-architecting the display system in U-Boot. What I
was suggesting was a way to architect Tegra-specific display driver code
to make it reusable rather than duplicate display controller programming
for each new generation, while the hardware has remained mostly the
same.

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

I don't think the driver model is a good fit here. Abstracting a display
port isn't very useful in itself because users don't really care about
the type of display, they only care about it being a display. So if you
want to usefully abstract you'd do it at a higher level, such as display
or screen. Then you have a generic object which users can use to put up
a framebuffer onto a physical screen.

SOR is an even worse abstraction because it's completely Tegra-specific
and other SoCs will have completely different ways of providing the same
types of output. You'll end up with a uclass containing a single
implementation.

So, to reiterate, the above wasn't meant to be a generic abstraction for
a U-Boot-wide display framework, but rather a suggestion on how the
Tegra driver could internally be structured in order to avoid code
duplication.

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

People asked for my opinion, so I shared. If you prefer code duplication
over a properly architected driver that's of course your prerogative.

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/20150824/550a0a6f/attachment.sig>

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

[Simon Glass]

+Nikita

Hi Thierry,

On 24 August 2015 at 04:12, Thierry Reding <treding@nvidia.com> wrote:
> On Fri, Aug 21, 2015 at 06:37:37PM -0600, Simon Glass wrote:
> [...]
>> 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?
>
> I didn't suggest re-architecting the display system in U-Boot. What I
> was suggesting was a way to architect Tegra-specific display driver code
> to make it reusable rather than duplicate display controller programming
> for each new generation, while the hardware has remained mostly the
> same.

OK, I misunderstood.

>
>> > 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.
>
> I don't think the driver model is a good fit here. Abstracting a display
> port isn't very useful in itself because users don't really care about
> the type of display, they only care about it being a display. So if you
> want to usefully abstract you'd do it at a higher level, such as display
> or screen. Then you have a generic object which users can use to put up
> a framebuffer onto a physical screen.

I think you are referring to the lcd/video interface. If so, this is
already fairly well defined, but lcd and video should be merged, and a
uclass could be added. Nikita Kiryanov has done quite a bit of work on
the merging side.

But I still think there is value in a low-level abstraction too.
Function pointers indicate that there is an interface that can be used
by multiple drivers, and that is what driver model is for. See
displayport.h for an attempt at this. We can of course consider
expanding the display port uclass to encompass panels in general. I
was reluctant to do that with a sample size of one. Here is the
current interface:

/**
 * display_port_read_edid() - Read information from EDID
 *
 * @dev:        Device to read from
 * @buf:        Buffer to read into (should be EDID_SIZE bytes)
 * @buf_size:   Buffer size (should be EDID_SIZE)
 * @return number of bytes read, <=0 for error
 */
int display_port_read_edid(struct udevice *dev, u8 *buf, int buf_size);

/**
 * display_port_enable() - Enable a display port device
 *
 * @dev:        Device to enable
 * @panel_bpp:  Number of bits per pixel for panel
 * @timing:     Display timings
 * @return 0 if OK, -ve on error
 */
int display_port_enable(struct udevice *dev, int panel_bpp,
                        const struct display_timing *timing);

>
> SOR is an even worse abstraction because it's completely Tegra-specific
> and other SoCs will have completely different ways of providing the same
> types of output. You'll end up with a uclass containing a single
> implementation.

But if it is a single implementation why do you need to add function
pointers? It would just be a normal call in that case. I'm not
suggesting we add uclasses with no generic use.

>
> So, to reiterate, the above wasn't meant to be a generic abstraction for
> a U-Boot-wide display framework, but rather a suggestion on how the
> Tegra driver could internally be structured in order to avoid code
> duplication.
>
>> > 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...
>
> People asked for my opinion, so I shared. If you prefer code duplication
> over a properly architected driver that's of course your prerogative.

I am wondering if the problem here is just that I misunderstood your
intent. How about:

- the display controller code (display.c) should be common across all Tegra SoCs
- the code (which was merged 3 years ago) should move to use the new
device tree bindings (as does tegra124 display support)

What am I missing?

Let's see what Marcel is able to do here.

Regards,
Simon

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

[Thierry Reding]

On Mon, Aug 24, 2015 at 10:58:48AM -0600, Simon Glass wrote:
> +Nikita
> 
> Hi Thierry,
> 
> On 24 August 2015 at 04:12, Thierry Reding <treding@nvidia.com> wrote:
> > On Fri, Aug 21, 2015 at 06:37:37PM -0600, Simon Glass wrote:
> > [...]
> >> 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?
> >
> > I didn't suggest re-architecting the display system in U-Boot. What I
> > was suggesting was a way to architect Tegra-specific display driver code
> > to make it reusable rather than duplicate display controller programming
> > for each new generation, while the hardware has remained mostly the
> > same.
> 
> OK, I misunderstood.
> 
> >
> >> > 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.
> >
> > I don't think the driver model is a good fit here. Abstracting a display
> > port isn't very useful in itself because users don't really care about
> > the type of display, they only care about it being a display. So if you
> > want to usefully abstract you'd do it at a higher level, such as display
> > or screen. Then you have a generic object which users can use to put up
> > a framebuffer onto a physical screen.
> 
> I think you are referring to the lcd/video interface. If so, this is
> already fairly well defined, but lcd and video should be merged, and a
> uclass could be added. Nikita Kiryanov has done quite a bit of work on
> the merging side.
> 
> But I still think there is value in a low-level abstraction too.
> Function pointers indicate that there is an interface that can be used
> by multiple drivers, and that is what driver model is for. See
> displayport.h for an attempt at this. We can of course consider
> expanding the display port uclass to encompass panels in general. I
> was reluctant to do that with a sample size of one. Here is the
> current interface:
> 
> /**
>  * display_port_read_edid() - Read information from EDID
>  *
>  * @dev:        Device to read from
>  * @buf:        Buffer to read into (should be EDID_SIZE bytes)
>  * @buf_size:   Buffer size (should be EDID_SIZE)
>  * @return number of bytes read, <=0 for error
>  */
> int display_port_read_edid(struct udevice *dev, u8 *buf, int buf_size);
> 
> /**
>  * display_port_enable() - Enable a display port device
>  *
>  * @dev:        Device to enable
>  * @panel_bpp:  Number of bits per pixel for panel
>  * @timing:     Display timings
>  * @return 0 if OK, -ve on error
>  */
> int display_port_enable(struct udevice *dev, int panel_bpp,
>                         const struct display_timing *timing);

Both of these really aren't specific to DisplayPort. A DSI or HDMI input
also wants to be enabled or have its EDID queried. Well, EDID may not be
available on most DSI panels, so I think this particular abstraction
should be slightly higher-level. What users are interested in isn't the
EDID information, but the content therein. So I think a better way to
return this type of information is by generating a list of modes (or a
single one) given a display output device.

And once you have that abstraction it becomes useless to abstract the
various types of outputs, because DisplayPort, LVDS, HDMI, DSI, etc.
will all behave the same.

> > SOR is an even worse abstraction because it's completely Tegra-specific
> > and other SoCs will have completely different ways of providing the same
> > types of output. You'll end up with a uclass containing a single
> > implementation.
> 
> But if it is a single implementation why do you need to add function
> pointers? It would just be a normal call in that case. I'm not
> suggesting we add uclasses with no generic use.

The function pointers are there to allow the display driver to call into
the different output drivers. Generally on Tegra what you do to scan out
a framebuffer is roughly this:

	1) setup display controller to drive a specified display mode
	2) enable a window to scan out a given framebuffer
	3) setup an output driver to take input from the display
	   controller and push it over the wire

1) and 2) will be the same no matter what output you use. 3) is specific
to the type of output, but can be done with the same software interface.

The function pointers help in implementing 3) using the same abstraction
which I called tegra_output. A driver for HDMI would implement this in
one way, while the driver for DSI would implement it in another. For SOR
you would have yet another implementation. But the display driver itself
would be able to treat them the same way.

> > So, to reiterate, the above wasn't meant to be a generic abstraction for
> > a U-Boot-wide display framework, but rather a suggestion on how the
> > Tegra driver could internally be structured in order to avoid code
> > duplication.
> >
> >> > 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...
> >
> > People asked for my opinion, so I shared. If you prefer code duplication
> > over a properly architected driver that's of course your prerogative.
> 
> I am wondering if the problem here is just that I misunderstood your
> intent. How about:
> 
> - the display controller code (display.c) should be common across all Tegra SoCs
> - the code (which was merged 3 years ago) should move to use the new
> device tree bindings (as does tegra124 display support)
> 
> What am I missing?

Sounds good to me. My suggestions were targetted at how to decouple some
of the code to allow both the RGB (for the existing Tegra20 support) and
SOR (for the existing Tegra124 support) outputs to be used, depending on
the particular use-case.

And yes, this can all be driven from DT. The driver should be able to
parse the "primary" output from DT using information that's available
(such as which outputs are enabled) and some heuristics in case multiple
outputs are enabled (DSI or RGB and HDMI for example). In all cases that
I know of the internal panel (RGB, DSI, eDP) will be the primary one, so
giving those priority over HDMI should always give you a sane default
configuration.

Once you have the primary output you can query what mode it should drive
(using static display mode information from an internal panel, or
preferably EDID), allocate an appropriately sized framebuffer, set the
mode and scan out that buffer.

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/20150825/ce477939/attachment.sig>

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

[Simon Glass]

Hi Thierry,

On 25 August 2015 at 05:02, Thierry Reding <treding@nvidia.com> wrote:
> On Mon, Aug 24, 2015 at 10:58:48AM -0600, Simon Glass wrote:
>> +Nikita
>>
>> Hi Thierry,
>>
>> On 24 August 2015 at 04:12, Thierry Reding <treding@nvidia.com> wrote:
>> > On Fri, Aug 21, 2015 at 06:37:37PM -0600, Simon Glass wrote:
>> > [...]
>> >> 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?
>> >
>> > I didn't suggest re-architecting the display system in U-Boot. What I
>> > was suggesting was a way to architect Tegra-specific display driver code
>> > to make it reusable rather than duplicate display controller programming
>> > for each new generation, while the hardware has remained mostly the
>> > same.
>>
>> OK, I misunderstood.
>>
>> >
>> >> > 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.
>> >
>> > I don't think the driver model is a good fit here. Abstracting a display
>> > port isn't very useful in itself because users don't really care about
>> > the type of display, they only care about it being a display. So if you
>> > want to usefully abstract you'd do it at a higher level, such as display
>> > or screen. Then you have a generic object which users can use to put up
>> > a framebuffer onto a physical screen.
>>
>> I think you are referring to the lcd/video interface. If so, this is
>> already fairly well defined, but lcd and video should be merged, and a
>> uclass could be added. Nikita Kiryanov has done quite a bit of work on
>> the merging side.
>>
>> But I still think there is value in a low-level abstraction too.
>> Function pointers indicate that there is an interface that can be used
>> by multiple drivers, and that is what driver model is for. See
>> displayport.h for an attempt at this. We can of course consider
>> expanding the display port uclass to encompass panels in general. I
>> was reluctant to do that with a sample size of one. Here is the
>> current interface:
>>
>> /**
>>  * display_port_read_edid() - Read information from EDID
>>  *
>>  * @dev:        Device to read from
>>  * @buf:        Buffer to read into (should be EDID_SIZE bytes)
>>  * @buf_size:   Buffer size (should be EDID_SIZE)
>>  * @return number of bytes read, <=0 for error
>>  */
>> int display_port_read_edid(struct udevice *dev, u8 *buf, int buf_size);
>>
>> /**
>>  * display_port_enable() - Enable a display port device
>>  *
>>  * @dev:        Device to enable
>>  * @panel_bpp:  Number of bits per pixel for panel
>>  * @timing:     Display timings
>>  * @return 0 if OK, -ve on error
>>  */
>> int display_port_enable(struct udevice *dev, int panel_bpp,
>>                         const struct display_timing *timing);
>
> Both of these really aren't specific to DisplayPort. A DSI or HDMI input
> also wants to be enabled or have its EDID queried. Well, EDID may not be
> available on most DSI panels, so I think this particular abstraction
> should be slightly higher-level. What users are interested in isn't the
> EDID information, but the content therein. So I think a better way to
> return this type of information is by generating a list of modes (or a
> single one) given a display output device.

That sounds reasonable.

>
> And once you have that abstraction it becomes useless to abstract the
> various types of outputs, because DisplayPort, LVDS, HDMI, DSI, etc.
> will all behave the same.

OK so once we have another user we should rename this to something
like VIDEOPORT...

>
>> > SOR is an even worse abstraction because it's completely Tegra-specific
>> > and other SoCs will have completely different ways of providing the same
>> > types of output. You'll end up with a uclass containing a single
>> > implementation.
>>
>> But if it is a single implementation why do you need to add function
>> pointers? It would just be a normal call in that case. I'm not
>> suggesting we add uclasses with no generic use.
>
> The function pointers are there to allow the display driver to call into
> the different output drivers. Generally on Tegra what you do to scan out
> a framebuffer is roughly this:
>
>         1) setup display controller to drive a specified display mode
>         2) enable a window to scan out a given framebuffer
>         3) setup an output driver to take input from the display
>            controller and push it over the wire
>
> 1) and 2) will be the same no matter what output you use. 3) is specific
> to the type of output, but can be done with the same software interface.
>
> The function pointers help in implementing 3) using the same abstraction
> which I called tegra_output. A driver for HDMI would implement this in
> one way, while the driver for DSI would implement it in another. For SOR
> you would have yet another implementation. But the display driver itself
> would be able to treat them the same way.

OK then I think we should create a new video output uclass for this
purpose. We then end up with a high-level 'interface' uclass (hdmi,
displayport, etc.) and a low-level 'video transport' uclass.

>
>> > So, to reiterate, the above wasn't meant to be a generic abstraction for
>> > a U-Boot-wide display framework, but rather a suggestion on how the
>> > Tegra driver could internally be structured in order to avoid code
>> > duplication.
>> >
>> >> > 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...
>> >
>> > People asked for my opinion, so I shared. If you prefer code duplication
>> > over a properly architected driver that's of course your prerogative.
>>
>> I am wondering if the problem here is just that I misunderstood your
>> intent. How about:
>>
>> - the display controller code (display.c) should be common across all Tegra SoCs
>> - the code (which was merged 3 years ago) should move to use the new
>> device tree bindings (as does tegra124 display support)
>>
>> What am I missing?
>
> Sounds good to me. My suggestions were targetted at how to decouple some
> of the code to allow both the RGB (for the existing Tegra20 support) and
> SOR (for the existing Tegra124 support) outputs to be used, depending on
> the particular use-case.
>
> And yes, this can all be driven from DT. The driver should be able to
> parse the "primary" output from DT using information that's available
> (such as which outputs are enabled) and some heuristics in case multiple
> outputs are enabled (DSI or RGB and HDMI for example). In all cases that
> I know of the internal panel (RGB, DSI, eDP) will be the primary one, so
> giving those priority over HDMI should always give you a sane default
> configuration.
>
> Once you have the primary output you can query what mode it should drive
> (using static display mode information from an internal panel, or
> preferably EDID), allocate an appropriately sized framebuffer, set the
> mode and scan out that buffer.

OK, sounds good. I'm not yet sure who is planning to work on this and
I would much prefer to do it with another SoC uses the same framework.
It's normally a bad idea to design a general purpose framework with a
sample size of 1!

Regards,
Simon

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

[Thierry Reding]

On Tue, Aug 25, 2015 at 10:03:13AM -0600, Simon Glass wrote:
> On 25 August 2015 at 05:02, Thierry Reding <treding@nvidia.com> wrote:
> > On Mon, Aug 24, 2015 at 10:58:48AM -0600, Simon Glass wrote:
> >> On 24 August 2015 at 04:12, Thierry Reding <treding@nvidia.com> wrote:
[...]
> >> > SOR is an even worse abstraction because it's completely Tegra-specific
> >> > and other SoCs will have completely different ways of providing the same
> >> > types of output. You'll end up with a uclass containing a single
> >> > implementation.
> >>
> >> But if it is a single implementation why do you need to add function
> >> pointers? It would just be a normal call in that case. I'm not
> >> suggesting we add uclasses with no generic use.
> >
> > The function pointers are there to allow the display driver to call into
> > the different output drivers. Generally on Tegra what you do to scan out
> > a framebuffer is roughly this:
> >
> >         1) setup display controller to drive a specified display mode
> >         2) enable a window to scan out a given framebuffer
> >         3) setup an output driver to take input from the display
> >            controller and push it over the wire
> >
> > 1) and 2) will be the same no matter what output you use. 3) is specific
> > to the type of output, but can be done with the same software interface.
> >
> > The function pointers help in implementing 3) using the same abstraction
> > which I called tegra_output. A driver for HDMI would implement this in
> > one way, while the driver for DSI would implement it in another. For SOR
> > you would have yet another implementation. But the display driver itself
> > would be able to treat them the same way.
> 
> OK then I think we should create a new video output uclass for this
> purpose. We then end up with a high-level 'interface' uclass (hdmi,
> displayport, etc.) and a low-level 'video transport' uclass.

I don't understand. This is all internal to the display driver, why
should it need to bother with uclasses?

> >> > So, to reiterate, the above wasn't meant to be a generic abstraction for
> >> > a U-Boot-wide display framework, but rather a suggestion on how the
> >> > Tegra driver could internally be structured in order to avoid code
> >> > duplication.
> >> >
> >> >> > 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...
> >> >
> >> > People asked for my opinion, so I shared. If you prefer code duplication
> >> > over a properly architected driver that's of course your prerogative.
> >>
> >> I am wondering if the problem here is just that I misunderstood your
> >> intent. How about:
> >>
> >> - the display controller code (display.c) should be common across all Tegra SoCs
> >> - the code (which was merged 3 years ago) should move to use the new
> >> device tree bindings (as does tegra124 display support)
> >>
> >> What am I missing?
> >
> > Sounds good to me. My suggestions were targetted at how to decouple some
> > of the code to allow both the RGB (for the existing Tegra20 support) and
> > SOR (for the existing Tegra124 support) outputs to be used, depending on
> > the particular use-case.
> >
> > And yes, this can all be driven from DT. The driver should be able to
> > parse the "primary" output from DT using information that's available
> > (such as which outputs are enabled) and some heuristics in case multiple
> > outputs are enabled (DSI or RGB and HDMI for example). In all cases that
> > I know of the internal panel (RGB, DSI, eDP) will be the primary one, so
> > giving those priority over HDMI should always give you a sane default
> > configuration.
> >
> > Once you have the primary output you can query what mode it should drive
> > (using static display mode information from an internal panel, or
> > preferably EDID), allocate an appropriately sized framebuffer, set the
> > mode and scan out that buffer.
> 
> OK, sounds good. I'm not yet sure who is planning to work on this and
> I would much prefer to do it with another SoC uses the same framework.
> It's normally a bad idea to design a general purpose framework with a
> sample size of 1!

Again, this is about adding infrastructure to the Tegra display driver
to allow code reuse. Why would this need to be done in conjunction with
other SoCs?

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/20150826/35bfc5d9/attachment.sig>

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

[Simon Glass]

Hi Thierry,

On 26 August 2015 at 00:29, Thierry Reding <treding@nvidia.com> wrote:
> On Tue, Aug 25, 2015 at 10:03:13AM -0600, Simon Glass wrote:
>> On 25 August 2015 at 05:02, Thierry Reding <treding@nvidia.com> wrote:
>> > On Mon, Aug 24, 2015 at 10:58:48AM -0600, Simon Glass wrote:
>> >> On 24 August 2015 at 04:12, Thierry Reding <treding@nvidia.com> wrote:
> [...]
>> >> > SOR is an even worse abstraction because it's completely Tegra-specific
>> >> > and other SoCs will have completely different ways of providing the same
>> >> > types of output. You'll end up with a uclass containing a single
>> >> > implementation.
>> >>
>> >> But if it is a single implementation why do you need to add function
>> >> pointers? It would just be a normal call in that case. I'm not
>> >> suggesting we add uclasses with no generic use.
>> >
>> > The function pointers are there to allow the display driver to call into
>> > the different output drivers. Generally on Tegra what you do to scan out
>> > a framebuffer is roughly this:
>> >
>> >         1) setup display controller to drive a specified display mode
>> >         2) enable a window to scan out a given framebuffer
>> >         3) setup an output driver to take input from the display
>> >            controller and push it over the wire
>> >
>> > 1) and 2) will be the same no matter what output you use. 3) is specific
>> > to the type of output, but can be done with the same software interface.
>> >
>> > The function pointers help in implementing 3) using the same abstraction
>> > which I called tegra_output. A driver for HDMI would implement this in
>> > one way, while the driver for DSI would implement it in another. For SOR
>> > you would have yet another implementation. But the display driver itself
>> > would be able to treat them the same way.
>>
>> OK then I think we should create a new video output uclass for this
>> purpose. We then end up with a high-level 'interface' uclass (hdmi,
>> displayport, etc.) and a low-level 'video transport' uclass.
>
> I don't understand. This is all internal to the display driver, why
> should it need to bother with uclasses?

One of the design goals of driver model is to expose these interfaces
so that the structure of devices can be seen. It hopefully promotes
code reuse and better software architecture since more thought goes
into exactly where these interfaces should be drawn, and whether they
can be shared across different drivers for the same subsystem.

Driver model was created to expose and standardise the various
interfaces in U-Boot between generic code (or even arch-specific code)
and the drivers it uses. This doesn't just apply at the top level -
e.g. pinctrl is arguably something that could be dealt with internally
by each driver, but we choose to make it its own first-class
subsystem. For Ethernet we have a PHY library to promote code reuse.

I'm sure there are merits on each side but I think the default
position should be that whenever an interface with function methods is
added we should consider it a new uclass. This is particularly true
when we have a device tree child since we want to main that tree
structure in driver model so that nodes correspond to devices.

I'm sure there are exceptions where this does not make sense.

>
>> >> > So, to reiterate, the above wasn't meant to be a generic abstraction for
>> >> > a U-Boot-wide display framework, but rather a suggestion on how the
>> >> > Tegra driver could internally be structured in order to avoid code
>> >> > duplication.
>> >> >
>> >> >> > 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...
>> >> >
>> >> > People asked for my opinion, so I shared. If you prefer code duplication
>> >> > over a properly architected driver that's of course your prerogative.
>> >>
>> >> I am wondering if the problem here is just that I misunderstood your
>> >> intent. How about:
>> >>
>> >> - the display controller code (display.c) should be common across all Tegra SoCs
>> >> - the code (which was merged 3 years ago) should move to use the new
>> >> device tree bindings (as does tegra124 display support)
>> >>
>> >> What am I missing?
>> >
>> > Sounds good to me. My suggestions were targetted at how to decouple some
>> > of the code to allow both the RGB (for the existing Tegra20 support) and
>> > SOR (for the existing Tegra124 support) outputs to be used, depending on
>> > the particular use-case.
>> >
>> > And yes, this can all be driven from DT. The driver should be able to
>> > parse the "primary" output from DT using information that's available
>> > (such as which outputs are enabled) and some heuristics in case multiple
>> > outputs are enabled (DSI or RGB and HDMI for example). In all cases that
>> > I know of the internal panel (RGB, DSI, eDP) will be the primary one, so
>> > giving those priority over HDMI should always give you a sane default
>> > configuration.
>> >
>> > Once you have the primary output you can query what mode it should drive
>> > (using static display mode information from an internal panel, or
>> > preferably EDID), allocate an appropriately sized framebuffer, set the
>> > mode and scan out that buffer.
>>
>> OK, sounds good. I'm not yet sure who is planning to work on this and
>> I would much prefer to do it with another SoC uses the same framework.
>> It's normally a bad idea to design a general purpose framework with a
>> sample size of 1!
>
> Again, this is about adding infrastructure to the Tegra display driver
> to allow code reuse. Why would this need to be done in conjunction with
> other SoCs?

It seems like the operations you are proposing would be the same on
any SoC with this hardware.

Regards,
Simon

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

[Marcel Ziswiler]

On 22 Aug 2015 02:38, Simon Glass <sjg@chromium.org> wrote:

> Please take a look at tegra124's display driver for an
> example.

You mean that second copy of the existing Tegra stuff I find myself now certainly in the middle of having to clean up? Thank you!

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

[Marcel Ziswiler]

On 21 Aug 2015 11:29, Thierry Reding <treding@nvidia.com> wrote:

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

I do not oppose however I doubt this has ever been the case. At least for the Tegras this is far from the truth.

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

I'm not even that sure whether using device trees in U-Boot is really that smart at all especially considering SPL and such. But that's a different thematic...

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

Exactly.

> 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 certainly give this a try on our hardware however most NVIDIA boards are far or at least were far from public so I can't test those as I don't have any access.

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

OK, fair enough.

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

Me wondering how that came along then?

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

Agreed, just wondering about the lack of any documentation thereof.

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

As mentioned before my doubts go as deep as the actual functional split thereof.

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

I also doubt finding that kind of time. My goal was simply to make T30 usable to the same extend as the T20 before it gets completely lost.

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

[Thierry Reding]

On Mon, Aug 24, 2015 at 10:03:35PM +0000, Marcel Ziswiler wrote:
> On 21 Aug 2015 11:29, Thierry Reding <treding@nvidia.com> wrote:
> > 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 certainly give this a try on our hardware however most NVIDIA
> boards are far or at least were far from public so I can't test those
> as I don't have any access.

I have access to most boards, so I can help with the testing. Tom and
Stephen may be able to help as well, so we can spread the load. I'm
fairly confident, though, that if we get things tested on one board per
Tegra generation it will work fine on other boards of the same
generation as well. There isn't all that much difference between boards
(at least as far as U-Boot cares).

> > 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.
> 
> Agreed, just wondering about the lack of any documentation thereof.

I don't understand this comment. The display controller is fairly well
documented in the TRM.

> > 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.
> 
> As mentioned before my doubts go as deep as the actual functional split thereof.

Which functional split are you referring to?

> > 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.
> 
> I also doubt finding that kind of time. My goal was simply to make T30
> usable to the same extend as the T20 before it gets completely lost.

You don't have to do all of the above to get Tegra30 to the same point
where Tegra20 is now. Perhaps a good middle-ground for now would be to
simply avoid duplicating the display driver completely and rather look
at reusing the Tegra20 code on both Tegra20 and Tegra30. That would be
not making things worse and should get you what you want.

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/20150826/aac5f182/attachment.sig>

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

[Simon Glass]

Hi,

On 26 August 2015 at 00:39, Thierry Reding <treding@nvidia.com> wrote:
> On Mon, Aug 24, 2015 at 10:03:35PM +0000, Marcel Ziswiler wrote:
>> On 21 Aug 2015 11:29, Thierry Reding <treding@nvidia.com> wrote:
>> > 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 certainly give this a try on our hardware however most NVIDIA
>> boards are far or at least were far from public so I can't test those
>> as I don't have any access.
>
> I have access to most boards, so I can help with the testing. Tom and
> Stephen may be able to help as well, so we can spread the load. I'm
> fairly confident, though, that if we get things tested on one board per
> Tegra generation it will work fine on other boards of the same
> generation as well. There isn't all that much difference between boards
> (at least as far as U-Boot cares).
>
>> > 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.
>>
>> Agreed, just wondering about the lack of any documentation thereof.
>
> I don't understand this comment. The display controller is fairly well
> documented in the TRM.
>
>> > 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.
>>
>> As mentioned before my doubts go as deep as the actual functional split thereof.
>
> Which functional split are you referring to?
>
>> > 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.
>>
>> I also doubt finding that kind of time. My goal was simply to make T30
>> usable to the same extend as the T20 before it gets completely lost.
>
> You don't have to do all of the above to get Tegra30 to the same point
> where Tegra20 is now. Perhaps a good middle-ground for now would be to
> simply avoid duplicating the display driver completely and rather look
> at reusing the Tegra20 code on both Tegra20 and Tegra30. That would be
> not making things worse and should get you what you want.

If you can make this work I can sign up for moving Tegra124 over.

Regards,
Simon

[U-Boot] [PATCH 0/5] arm: tegra: apalis/colibri_t30: video: add display driver

[Marcel Ziswiler]

On popular request this series integrates the display driver from T20
to work on T30 as well and enables it for our Apalis/Colibri T30
computer/system on modules. Enjoy.

Marcel Ziswiler (5):
  arm: tegra20: video: rename display header ifdef gating
  arm: tegra20: video: ifdef gate hard-coded ugly Tegra20 pin muxing
  arm: tegra30: video: integrate display driver for t30
  colibri_t30: enable display driver
  apalis_t30: enable display driver

 arch/arm/dts/tegra30-apalis.dts                    |  29 ++
 arch/arm/dts/tegra30-colibri.dts                   |  29 ++
 arch/arm/dts/tegra30.dtsi                          | 104 ++++++
 arch/arm/include/asm/arch-tegra20/display.h        |   6 +-
 arch/arm/include/asm/arch-tegra30/display.h        |  13 +
 arch/arm/include/asm/arch-tegra30/pwm.h            |  14 +
 arch/arm/mach-tegra/tegra30/Makefile               |   6 +-
 arch/arm/mach-tegra/tegra30/display.c              | 394 +++++++++++++++++++++
 .../toradex/apalis_t30/pinmux-config-apalis_t30.h  |   9 +-
 .../colibri_t30/pinmux-config-colibri_t30.h        |   8 +-
 drivers/video/tegra.c                              |   2 +
 include/configs/apalis_t30.h                       |  10 +
 include/configs/colibri_t30.h                      |  10 +
 include/configs/tegra30-common.h                   |   3 +
 14 files changed, 625 insertions(+), 12 deletions(-)
 create mode 100644 arch/arm/include/asm/arch-tegra30/display.h
 create mode 100644 arch/arm/include/asm/arch-tegra30/pwm.h
 create mode 100644 arch/arm/mach-tegra/tegra30/display.c

-- 
2.4.3

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

[Marcel Ziswiler]

On popular request make the display driver from T20 work on T30 as
well. Turned out to be quite straight forward. However a few notes
about some things encountered during porting: Of course the T30 device
tree was completely missing host1x as well as PWM support but it turns
out this can simply be copied from T20 supplementing some tegra30-
compatible nodes here and there while comparing it with the Linux
device tree includes.

Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
---
The older Chromium U-Boot 2011.06 which to my knowledge was the only
prior attempt at enabling a display driver for T30 for whatever reason
got some clocking stuff mixed up. Turns out at least for a single
display controller T20 and T30 can be clocked quite similar.

 arch/arm/dts/tegra30.dtsi                   | 104 ++++++++
 arch/arm/include/asm/arch-tegra30/display.h |  13 +
 arch/arm/include/asm/arch-tegra30/pwm.h     |  14 +
 arch/arm/mach-tegra/tegra30/Makefile        |   6 +-
 arch/arm/mach-tegra/tegra30/display.c       | 394 ++++++++++++++++++++++++++++
 include/configs/tegra30-common.h            |   3 +
 6 files changed, 533 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/include/asm/arch-tegra30/display.h
 create mode 100644 arch/arm/include/asm/arch-tegra30/pwm.h
 create mode 100644 arch/arm/mach-tegra/tegra30/display.c

diff --git a/arch/arm/dts/tegra30.dtsi b/arch/arm/dts/tegra30.dtsi
index 5ea7e34..7669b27 100644
--- a/arch/arm/dts/tegra30.dtsi
+++ b/arch/arm/dts/tegra30.dtsi
@@ -8,6 +8,104 @@
 	compatible = "nvidia,tegra30";
 	interrupt-parent = <&intc>;
 
+	host1x {
+		compatible = "nvidia,tegra30-host1x", "simple-bus";
+		reg = <0x50000000 0x00024000>;
+		interrupts = <0 65 0x04   /* mpcore syncpt */
+			      0 67 0x04>; /* mpcore general */
+		status = "disabled";
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		ranges = <0x54000000 0x54000000 0x04000000>;
+
+		/* video-encoding/decoding */
+		mpe {
+			reg = <0x54040000 0x00040000>;
+			interrupts = <0 68 0x04>;
+			status = "disabled";
+		};
+
+		/* video input */
+		vi {
+			reg = <0x54080000 0x00040000>;
+			interrupts = <0 69 0x04>;
+			status = "disabled";
+		};
+
+		/* EPP */
+		epp {
+			reg = <0x540c0000 0x00040000>;
+			interrupts = <0 70 0x04>;
+			status = "disabled";
+		};
+
+		/* ISP */
+		isp {
+			reg = <0x54100000 0x00040000>;
+			interrupts = <0 71 0x04>;
+			status = "disabled";
+		};
+
+		/* 2D engine */
+		gr2d {
+			reg = <0x54140000 0x00040000>;
+			interrupts = <0 72 0x04>;
+			status = "disabled";
+		};
+
+		/* 3D engine */
+		gr3d {
+			reg = <0x54180000 0x00040000>;
+			status = "disabled";
+		};
+
+		/* display controllers */
+		dc at 54200000 {
+			compatible = "nvidia,tegra30-dc", "nvidia,tegra20-dc";
+			reg = <0x54200000 0x00040000>;
+			interrupts = <0 73 0x04>;
+			status = "disabled";
+
+			rgb {
+				status = "disabled";
+			};
+		};
+
+		dc at 54240000 {
+			compatible = "nvidia,tegra30-dc";
+			reg = <0x54240000 0x00040000>;
+			interrupts = <0 74 0x04>;
+			status = "disabled";
+
+			rgb {
+				status = "disabled";
+			};
+		};
+
+		/* outputs */
+		hdmi {
+			compatible = "nvidia,tegra30-hdmi";
+			reg = <0x54280000 0x00040000>;
+			interrupts = <0 75 0x04>;
+			status = "disabled";
+		};
+
+		tvo {
+			compatible = "nvidia,tegra30-tvo";
+			reg = <0x542c0000 0x00040000>;
+			interrupts = <0 76 0x04>;
+			status = "disabled";
+		};
+
+		dsi {
+			compatible = "nvidia,tegra30-dsi";
+			reg = <0x54300000 0x00040000>;
+			status = "disabled";
+		};
+	};
+
 	intc: interrupt-controller at 50041000 {
 		compatible = "arm,cortex-a9-gic";
 		reg = <0x50041000 0x1000
@@ -271,6 +369,12 @@
 		status = "disabled";
 	};
 
+	pwm: pwm@7000a000 {
+		compatible = "nvidia,tegra30-pwm", "nvidia,tegra20-pwm";
+		reg = <0x7000a000 0x100>;
+		#pwm-cells = <2>;
+	};
+
 	spi at 7000d400 {
 		compatible = "nvidia,tegra30-slink", "nvidia,tegra20-slink";
 		reg = <0x7000d400 0x200>;
diff --git a/arch/arm/include/asm/arch-tegra30/display.h b/arch/arm/include/asm/arch-tegra30/display.h
new file mode 100644
index 0000000..6a46541
--- /dev/null
+++ b/arch/arm/include/asm/arch-tegra30/display.h
@@ -0,0 +1,13 @@
+/*
+ *  (C) Copyright 2010
+ *  NVIDIA Corporation <www.nvidia.com>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#ifndef __ASM_ARCH_TEGRA30_DISPLAY_H
+#define __ASM_ARCH_TEGRA30_DISPLAY_H
+
+#include <asm/arch-tegra20/display.h>
+
+#endif	/* __ASM_ARCH_TEGRA30_DISPLAY_H */
diff --git a/arch/arm/include/asm/arch-tegra30/pwm.h b/arch/arm/include/asm/arch-tegra30/pwm.h
new file mode 100644
index 0000000..98d24d0
--- /dev/null
+++ b/arch/arm/include/asm/arch-tegra30/pwm.h
@@ -0,0 +1,14 @@
+/*
+ * Tegra pulse width frequency modulator definitions
+ *
+ * Copyright (c) 2011 The Chromium OS Authors.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#ifndef __ASM_ARCH_TEGRA30_PWM_H
+#define __ASM_ARCH_TEGRA30_PWM_H
+
+#include <asm/arch-tegra/pwm.h>
+
+#endif	/* __ASM_ARCH_TEGRA30_PWM_H */
diff --git a/arch/arm/mach-tegra/tegra30/Makefile b/arch/arm/mach-tegra/tegra30/Makefile
index bc250de..ef1ceb1 100644
--- a/arch/arm/mach-tegra/tegra30/Makefile
+++ b/arch/arm/mach-tegra/tegra30/Makefile
@@ -14,6 +14,10 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
-obj-$(CONFIG_SPL_BUILD) += cpu.o
+ifdef CONFIG_SPL_BUILD
+obj-y	+= cpu.o
+else
+obj-$(CONFIG_VIDEO_TEGRA) += display.o
+endif
 
 obj-y	+= clock.o funcmux.o pinmux.o
diff --git a/arch/arm/mach-tegra/tegra30/display.c b/arch/arm/mach-tegra/tegra30/display.c
new file mode 100644
index 0000000..b7605ff
--- /dev/null
+++ b/arch/arm/mach-tegra/tegra30/display.c
@@ -0,0 +1,394 @@
+/*
+ *  (C) Copyright 2010
+ *  NVIDIA Corporation <www.nvidia.com>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <asm/io.h>
+#include <asm/arch/clock.h>
+#include <asm/arch/tegra.h>
+#include <asm/arch/display.h>
+#include <asm/arch-tegra/dc.h>
+#include <asm/arch-tegra/clk_rst.h>
+#include <asm/arch-tegra/timer.h>
+
+static struct fdt_disp_config config;
+
+static void update_window(struct dc_ctlr *dc, struct disp_ctl_win *win)
+{
+	unsigned h_dda, v_dda;
+	unsigned long val;
+
+	val = readl(&dc->cmd.disp_win_header);
+	val |= WINDOW_A_SELECT;
+	writel(val, &dc->cmd.disp_win_header);
+
+	writel(win->fmt, &dc->win.color_depth);
+
+	clrsetbits_le32(&dc->win.byte_swap, BYTE_SWAP_MASK,
+			BYTE_SWAP_NOSWAP << BYTE_SWAP_SHIFT);
+
+	val = win->out_x << H_POSITION_SHIFT;
+	val |= win->out_y << V_POSITION_SHIFT;
+	writel(val, &dc->win.pos);
+
+	val = win->out_w << H_SIZE_SHIFT;
+	val |= win->out_h << V_SIZE_SHIFT;
+	writel(val, &dc->win.size);
+
+	val = (win->w * win->bpp / 8) << H_PRESCALED_SIZE_SHIFT;
+	val |= win->h << V_PRESCALED_SIZE_SHIFT;
+	writel(val, &dc->win.prescaled_size);
+
+	writel(0, &dc->win.h_initial_dda);
+	writel(0, &dc->win.v_initial_dda);
+
+	h_dda = (win->w * 0x1000) / max(win->out_w - 1, 1U);
+	v_dda = (win->h * 0x1000) / max(win->out_h - 1, 1U);
+
+	val = h_dda << H_DDA_INC_SHIFT;
+	val |= v_dda << V_DDA_INC_SHIFT;
+	writel(val, &dc->win.dda_increment);
+
+	writel(win->stride, &dc->win.line_stride);
+	writel(0, &dc->win.buf_stride);
+
+	val = WIN_ENABLE;
+	if (win->bpp < 24)
+		val |= COLOR_EXPAND;
+	writel(val, &dc->win.win_opt);
+
+	writel((unsigned long)win->phys_addr, &dc->winbuf.start_addr);
+	writel(win->x, &dc->winbuf.addr_h_offset);
+	writel(win->y, &dc->winbuf.addr_v_offset);
+
+	writel(0xff00, &dc->win.blend_nokey);
+	writel(0xff00, &dc->win.blend_1win);
+
+	val = GENERAL_ACT_REQ | WIN_A_ACT_REQ;
+	val |= GENERAL_UPDATE | WIN_A_UPDATE;
+	writel(val, &dc->cmd.state_ctrl);
+}
+
+static void write_pair(struct fdt_disp_config *config, int item, u32 *reg)
+{
+	writel(config->horiz_timing[item] |
+			(config->vert_timing[item] << 16), reg);
+}
+
+static int update_display_mode(struct dc_disp_reg *disp,
+		struct fdt_disp_config *config)
+{
+	unsigned long val;
+	unsigned long rate;
+	unsigned long div;
+
+	writel(0x0, &disp->disp_timing_opt);
+	write_pair(config, FDT_LCD_TIMING_REF_TO_SYNC, &disp->ref_to_sync);
+	write_pair(config, FDT_LCD_TIMING_SYNC_WIDTH, &disp->sync_width);
+	write_pair(config, FDT_LCD_TIMING_BACK_PORCH, &disp->back_porch);
+	write_pair(config, FDT_LCD_TIMING_FRONT_PORCH, &disp->front_porch);
+
+	writel(config->width | (config->height << 16), &disp->disp_active);
+
+	val = DE_SELECT_ACTIVE << DE_SELECT_SHIFT;
+	val |= DE_CONTROL_NORMAL << DE_CONTROL_SHIFT;
+	writel(val, &disp->data_enable_opt);
+
+	val = DATA_FORMAT_DF1P1C << DATA_FORMAT_SHIFT;
+	val |= DATA_ALIGNMENT_MSB << DATA_ALIGNMENT_SHIFT;
+	val |= DATA_ORDER_RED_BLUE << DATA_ORDER_SHIFT;
+	writel(val, &disp->disp_interface_ctrl);
+
+	/*
+	 * The pixel clock divider is in 7.1 format (where the bottom bit
+	 * represents 0.5). Here we calculate the divider needed to get from
+	 * the display clock (typically 600MHz) to the pixel clock. We round
+	 * up or down as requried.
+	 */
+	rate = clock_get_periph_rate(PERIPH_ID_DISP1, CLOCK_ID_CGENERAL);
+	div = ((rate * 2 + config->pixel_clock / 2) / config->pixel_clock) - 2;
+	debug("Display clock %lu, divider %lu\n", rate, div);
+
+	writel(0x00010001, &disp->shift_clk_opt);
+
+	val = PIXEL_CLK_DIVIDER_PCD1 << PIXEL_CLK_DIVIDER_SHIFT;
+	val |= div << SHIFT_CLK_DIVIDER_SHIFT;
+	writel(val, &disp->disp_clk_ctrl);
+
+	return 0;
+}
+
+/* Start up the display and turn on power to PWMs */
+static void basic_init(struct dc_cmd_reg *cmd)
+{
+	u32 val;
+
+	writel(0x00000100, &cmd->gen_incr_syncpt_ctrl);
+	writel(0x0000011a, &cmd->cont_syncpt_vsync);
+	writel(0x00000000, &cmd->int_type);
+	writel(0x00000000, &cmd->int_polarity);
+	writel(0x00000000, &cmd->int_mask);
+	writel(0x00000000, &cmd->int_enb);
+
+	val = PW0_ENABLE | PW1_ENABLE | PW2_ENABLE;
+	val |= PW3_ENABLE | PW4_ENABLE | PM0_ENABLE;
+	val |= PM1_ENABLE;
+	writel(val, &cmd->disp_pow_ctrl);
+
+	val = readl(&cmd->disp_cmd);
+	val |= CTRL_MODE_C_DISPLAY << CTRL_MODE_SHIFT;
+	writel(val, &cmd->disp_cmd);
+}
+
+static void basic_init_timer(struct dc_disp_reg *disp)
+{
+	writel(0x00000020, &disp->mem_high_pri);
+	writel(0x00000001, &disp->mem_high_pri_timer);
+}
+
+static const u32 rgb_enb_tab[PIN_REG_COUNT] = {
+	0x00000000,
+	0x00000000,
+	0x00000000,
+	0x00000000,
+};
+
+static const u32 rgb_polarity_tab[PIN_REG_COUNT] = {
+	0x00000000,
+	0x01000000,
+	0x00000000,
+	0x00000000,
+};
+
+static const u32 rgb_data_tab[PIN_REG_COUNT] = {
+	0x00000000,
+	0x00000000,
+	0x00000000,
+	0x00000000,
+};
+
+static const u32 rgb_sel_tab[PIN_OUTPUT_SEL_COUNT] = {
+	0x00000000,
+	0x00000000,
+	0x00000000,
+	0x00000000,
+	0x00210222,
+	0x00002200,
+	0x00020000,
+};
+
+static void rgb_enable(struct dc_com_reg *com)
+{
+	int i;
+
+	for (i = 0; i < PIN_REG_COUNT; i++) {
+		writel(rgb_enb_tab[i], &com->pin_output_enb[i]);
+		writel(rgb_polarity_tab[i], &com->pin_output_polarity[i]);
+		writel(rgb_data_tab[i], &com->pin_output_data[i]);
+	}
+
+	for (i = 0; i < PIN_OUTPUT_SEL_COUNT; i++)
+		writel(rgb_sel_tab[i], &com->pin_output_sel[i]);
+}
+
+static int setup_window(struct disp_ctl_win *win,
+			struct fdt_disp_config *config)
+{
+	win->x = 0;
+	win->y = 0;
+	win->w = config->width;
+	win->h = config->height;
+	win->out_x = 0;
+	win->out_y = 0;
+	win->out_w = config->width;
+	win->out_h = config->height;
+	win->phys_addr = config->frame_buffer;
+	win->stride = config->width * (1 << config->log2_bpp) / 8;
+	debug("%s: depth = %d\n", __func__, config->log2_bpp);
+	switch (config->log2_bpp) {
+	case 5:
+	case 24:
+		win->fmt = COLOR_DEPTH_R8G8B8A8;
+		win->bpp = 32;
+		break;
+	case 4:
+		win->fmt = COLOR_DEPTH_B5G6R5;
+		win->bpp = 16;
+		break;
+
+	default:
+		debug("Unsupported LCD bit depth");
+		return -1;
+	}
+
+	return 0;
+}
+
+struct fdt_disp_config *tegra_display_get_config(void)
+{
+	return config.valid ? &config : NULL;
+}
+
+static void debug_timing(const char *name, unsigned int timing[])
+{
+#ifdef DEBUG
+	int i;
+
+	debug("%s timing: ", name);
+	for (i = 0; i < FDT_LCD_TIMING_COUNT; i++)
+		debug("%d ", timing[i]);
+	debug("\n");
+#endif
+}
+
+/**
+ * Decode panel information from the fdt, according to a standard binding
+ *
+ * @param blob		fdt blob
+ * @param node		offset of fdt node to read from
+ * @param config	structure to store fdt config into
+ * @return 0 if ok, -ve on error
+ */
+static int tegra_decode_panel(const void *blob, int node,
+			      struct fdt_disp_config *config)
+{
+	int front, back, ref;
+
+	config->width = fdtdec_get_int(blob, node, "xres", -1);
+	config->height = fdtdec_get_int(blob, node, "yres", -1);
+	config->pixel_clock = fdtdec_get_int(blob, node, "clock", 0);
+	if (!config->pixel_clock || config->width == -1 ||
+			config->height == -1) {
+		debug("%s: Pixel parameters missing\n", __func__);
+		return -FDT_ERR_NOTFOUND;
+	}
+
+	back = fdtdec_get_int(blob, node, "left-margin", -1);
+	front = fdtdec_get_int(blob, node, "right-margin", -1);
+	ref = fdtdec_get_int(blob, node, "hsync-len", -1);
+	if ((back | front | ref) == -1) {
+		debug("%s: Horizontal parameters missing\n", __func__);
+		return -FDT_ERR_NOTFOUND;
+	}
+
+	/* Use a ref-to-sync of 1 always, and take this from the front porch */
+	config->horiz_timing[FDT_LCD_TIMING_REF_TO_SYNC] = 1;
+	config->horiz_timing[FDT_LCD_TIMING_SYNC_WIDTH] = ref;
+	config->horiz_timing[FDT_LCD_TIMING_BACK_PORCH] = back;
+	config->horiz_timing[FDT_LCD_TIMING_FRONT_PORCH] = front -
+		config->horiz_timing[FDT_LCD_TIMING_REF_TO_SYNC];
+	debug_timing("horiz", config->horiz_timing);
+
+	back = fdtdec_get_int(blob, node, "upper-margin", -1);
+	front = fdtdec_get_int(blob, node, "lower-margin", -1);
+	ref = fdtdec_get_int(blob, node, "vsync-len", -1);
+	if ((back | front | ref) == -1) {
+		debug("%s: Vertical parameters missing\n", __func__);
+		return -FDT_ERR_NOTFOUND;
+	}
+
+	config->vert_timing[FDT_LCD_TIMING_REF_TO_SYNC] = 1;
+	config->vert_timing[FDT_LCD_TIMING_SYNC_WIDTH] = ref;
+	config->vert_timing[FDT_LCD_TIMING_BACK_PORCH] = back;
+	config->vert_timing[FDT_LCD_TIMING_FRONT_PORCH] = front -
+		config->vert_timing[FDT_LCD_TIMING_REF_TO_SYNC];
+	debug_timing("vert", config->vert_timing);
+
+	return 0;
+}
+
+/**
+ * Decode the display controller information from the fdt.
+ *
+ * @param blob		fdt blob
+ * @param config	structure to store fdt config into
+ * @return 0 if ok, -ve on error
+ */
+static int tegra_display_decode_config(const void *blob,
+				       struct fdt_disp_config *config)
+{
+	int node, rgb;
+	int bpp, bit;
+
+	/* TODO: Support multiple controllers */
+	node = fdtdec_next_compatible(blob, 0, COMPAT_NVIDIA_TEGRA20_DC);
+	if (node < 0) {
+		debug("%s: Cannot find display controller node in fdt\n",
+		      __func__);
+		return node;
+	}
+	config->disp = (struct disp_ctlr *)fdtdec_get_addr(blob, node, "reg");
+	if (!config->disp) {
+		debug("%s: No display controller address\n", __func__);
+		return -1;
+	}
+
+	rgb = fdt_subnode_offset(blob, node, "rgb");
+
+	config->panel_node = fdtdec_lookup_phandle(blob, rgb, "nvidia,panel");
+	if (config->panel_node < 0) {
+		debug("%s: Cannot find panel information\n", __func__);
+		return -1;
+	}
+
+	if (tegra_decode_panel(blob, config->panel_node, config)) {
+		debug("%s: Failed to decode panel information\n", __func__);
+		return -1;
+	}
+
+	bpp = fdtdec_get_int(blob, config->panel_node, "nvidia,bits-per-pixel",
+			     -1);
+	bit = ffs(bpp) - 1;
+	if (bpp == (1 << bit))
+		config->log2_bpp = bit;
+	else
+		config->log2_bpp = bpp;
+	if (bpp == -1) {
+		debug("%s: Pixel bpp parameters missing\n", __func__);
+		return -FDT_ERR_NOTFOUND;
+	}
+	config->bpp = bpp;
+
+	config->valid = 1;	/* we have a valid configuration */
+
+	return 0;
+}
+
+int tegra_display_probe(const void *blob, void *default_lcd_base)
+{
+	struct disp_ctl_win window;
+	struct dc_ctlr *dc;
+
+	if (tegra_display_decode_config(blob, &config))
+		return -1;
+
+	config.frame_buffer = (u32)default_lcd_base;
+
+	dc = (struct dc_ctlr *)config.disp;
+
+	/*
+	 * A header file for clock constants was NAKed upstream.
+	 * TODO: Put this into the FDT and fdt_lcd struct when we have clock
+	 * support there
+	 */
+	clock_start_periph_pll(PERIPH_ID_HOST1X, CLOCK_ID_PERIPH,
+			       144 * 1000000);
+	clock_start_periph_pll(PERIPH_ID_DISP1, CLOCK_ID_CGENERAL,
+			       600 * 1000000);
+	basic_init(&dc->cmd);
+	basic_init_timer(&dc->disp);
+	rgb_enable(&dc->com);
+
+	if (config.pixel_clock)
+		update_display_mode(&dc->disp, &config);
+
+	if (setup_window(&window, &config))
+		return -1;
+
+	update_window(dc, &window);
+
+	return 0;
+}
diff --git a/include/configs/tegra30-common.h b/include/configs/tegra30-common.h
index 3e8e3c1..0360578 100644
--- a/include/configs/tegra30-common.h
+++ b/include/configs/tegra30-common.h
@@ -70,6 +70,9 @@
 #define CONFIG_SYS_SPL_MALLOC_START	0x80090000
 #define CONFIG_SPL_STACK		0x800ffffc
 
+/* Align LCD to 1MB boundary */
+#define CONFIG_LCD_ALIGNMENT	MMU_SECTION_SIZE
+
 /* For USB EHCI controller */
 #define CONFIG_EHCI_IS_TDI
 #define CONFIG_USB_EHCI_TXFIFO_THRESH	0x10
-- 
2.4.3

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

[Stephen Warren]

On 08/20/2015 05:29 AM, Marcel Ziswiler wrote:
> On popular request make the display driver from T20 work on T30 as
> well. Turned out to be quite straight forward. However a few notes
> about some things encountered during porting: Of course the T30 device
> tree was completely missing host1x as well as PWM support but it turns
> out this can simply be copied from T20 supplementing some tegra30-
> compatible nodes here and there while comparing it with the Linux
> device tree includes.

Hopefully the process was to copy the Linux Tegra30 DT verbatim? That's 
far more likely to yield a correct DT than copying the Tegra20 DT to 
Tegra30 and then patching it until it works. If this DT doesn't exactly 
match the Linux kernel, this needs to be fixed.

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

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