[PATCH fix for 3.19] ARM: dts: sun4i: Add simplefb node with de_fe0-de_be0-lcd0-hdmi pipeline

[PATCH fix for 3.19] ARM: dts: sun4i: Add simplefb node with de_fe0-de_be0-lcd0-hdmi pipeline

[Hans de Goede]

Hi Maxime,

As I already mailed you about yesterday as a headsup here is a fix for
the simplefb dt nodes introduced for sunxi in 3.19, I would like to get
this into 3.19 to avoid the need for carrying compatibility code for the old
nodes in u-boot (as 3.19 is going to be the first kernel release with sunxi
simplefb support).

>From the commit message of the attached patch:

###

Testing has shown that on sun4i the display backend engine does not have
deep enough fifo-s causing flickering / tearing in full-hd mode due to
fifo underruns. This can be avoided by letting the display frontend engine
do the dma from memory, and then letting it feed the data directly into
the backend unmodified, as the frontend does have deep enough fifo-s.

Note since u-boot-v2015.01 has been released using the de_be0-lcd0-hdmi
pipeline on sun4i, we need to keep that one around too (unfortunately).

###

I've send a patch to the u-boot mailinglist this morning to make u-boot
v2015.04 (and later) use a de_fe0-de_be0-lcd0-hdmi pipeline on sun4i thus
fixing the tearing / flickering.

Can you please queue this patch up as a fix for 3.19 ?

Thanks & Regards,

Hans

[PATCH] ARM: dts: sun4i: Add simplefb node with de_fe0-de_be0-lcd0-hdmi pipeline

[Hans de Goede]

Testing has shown that on sun4i the display backend engine does not have
deep enough fifo-s causing flickering / tearing in full-hd mode due to
fifo underruns. This can be avoided by letting the display frontend engine
do the dma from memory, and then letting it feed the data directly into
the backend unmodified, as the frontend does have deep enough fifo-s.

Note since u-boot-v2015.01 has been released using the de_be0-lcd0-hdmi
pipeline on sun4i, we need to keep that one around too (unfortunately).

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/sun4i-a10.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index f5e35b5..ccd60e3 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -44,6 +44,14 @@
 				 <&ahb_gates 44>;
 			status = "disabled";
 		};
+
+		framebuffer@1 {
+			compatible = "allwinner,simple-framebuffer", "simple-framebuffer";
+			allwinner,pipeline = "de_fe0-de_be0-lcd0-hdmi";
+			clocks = <&pll5 1>, <&ahb_gates 36>, <&ahb_gates 43>,
+				 <&ahb_gates 44>, <&ahb_gates 46>;
+			status = "disabled";
+		};
 	};
 
 	cpus {
-- 
2.1.0

Re: [PATCH] ARM: dts: sun4i: Add simplefb node with de_fe0-de_be0-lcd0-hdmi pipeline

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2147 bytes --]

Hi Hans,

On Mon, Jan 19, 2015 at 02:01:17PM +0100, Hans de Goede wrote:
> Testing has shown that on sun4i the display backend engine does not have
> deep enough fifo-s causing flickering / tearing in full-hd mode due to
> fifo underruns. This can be avoided by letting the display frontend engine
> do the dma from memory, and then letting it feed the data directly into
> the backend unmodified, as the frontend does have deep enough fifo-s.
> 
> Note since u-boot-v2015.01 has been released using the de_be0-lcd0-hdmi
> pipeline on sun4i, we need to keep that one around too (unfortunately).
> 
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/boot/dts/sun4i-a10.dtsi | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
> index f5e35b5..ccd60e3 100644
> --- a/arch/arm/boot/dts/sun4i-a10.dtsi
> +++ b/arch/arm/boot/dts/sun4i-a10.dtsi
> @@ -44,6 +44,14 @@
>  				 <&ahb_gates 44>;
>  			status = "disabled";
>  		};
> +
> +		framebuffer@1 {
> +			compatible = "allwinner,simple-framebuffer", "simple-framebuffer";
> +			allwinner,pipeline = "de_fe0-de_be0-lcd0-hdmi";
> +			clocks = <&pll5 1>, <&ahb_gates 36>, <&ahb_gates 43>,
> +				 <&ahb_gates 44>, <&ahb_gates 46>;
> +			status = "disabled";
> +		};

Thanks for this.

I do have a question though, maybe I'm missing something, but what
would prevent us from using the existing node already? The output
pipeline seems to be the same, at least for which output we
use. Wether we use or not the display frontend looks like an
implementation detail.

That or just having several strings in allwinner,pipeline.

If this simplefb binding is just going to be a collection of all the
various combinations we have, plus the legacy options, I'm a bit
worried that this is just going to be a hell to maintain, without any
way to remove it at compile time (if such feature is ever merged in
DTC).

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] ARM: dts: sun4i: Add simplefb node with de_fe0-de_be0-lcd0-hdmi pipeline

[Hans de Goede]

Hi,

On 19-01-15 14:20, Maxime Ripard wrote:
> Hi Hans,
>
> On Mon, Jan 19, 2015 at 02:01:17PM +0100, Hans de Goede wrote:
>> Testing has shown that on sun4i the display backend engine does not have
>> deep enough fifo-s causing flickering / tearing in full-hd mode due to
>> fifo underruns. This can be avoided by letting the display frontend engine
>> do the dma from memory, and then letting it feed the data directly into
>> the backend unmodified, as the frontend does have deep enough fifo-s.
>>
>> Note since u-boot-v2015.01 has been released using the de_be0-lcd0-hdmi
>> pipeline on sun4i, we need to keep that one around too (unfortunately).
>>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>   arch/arm/boot/dts/sun4i-a10.dtsi | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
>> index f5e35b5..ccd60e3 100644
>> --- a/arch/arm/boot/dts/sun4i-a10.dtsi
>> +++ b/arch/arm/boot/dts/sun4i-a10.dtsi
>> @@ -44,6 +44,14 @@
>>   				 <&ahb_gates 44>;
>>   			status = "disabled";
>>   		};
>> +
>> +		framebuffer@1 {
>> +			compatible = "allwinner,simple-framebuffer", "simple-framebuffer";
>> +			allwinner,pipeline = "de_fe0-de_be0-lcd0-hdmi";
>> +			clocks = <&pll5 1>, <&ahb_gates 36>, <&ahb_gates 43>,
>> +				 <&ahb_gates 44>, <&ahb_gates 46>;
>> +			status = "disabled";
>> +		};
>
> Thanks for this.
>
> I do have a question though, maybe I'm missing something, but what
> would prevent us from using the existing node already? The output
> pipeline seems to be the same, at least for which output we
> use. Wether we use or not the display frontend looks like an
> implementation detail.

The purpose of having a node per pipeline is to have the rights clocks
listed for the pipeline chosen. So the "de_fe0-de_be0-lcd0-hdmi" node
has one ahb_gates entry more so as to make sure the kernel does not gate
off the frontend.

The idea of having different nodes per pipeline is that when we get
support for more module clocks in place, as a preparation for kms
support, we can add the module clocks to the relevant nodes, to also
ensure the module clocks will not get turned off.

By having this info in the dtb, rather then in u-boot we can update
the clocks list as we add new clock nodes in the dtb, ensuring that
an older u-boot will keep working as we add support for module
clocks, etc.

> That or just having several strings in allwinner,pipeline.
>
> If this simplefb binding is just going to be a collection of all the
> various combinations we have, plus the legacy options, I'm a bit
> worried that this is just going to be a hell to maintain, without any
> way to remove it at compile time (if such feature is ever merged in
> DTC).

I can understand where you're coming from, I wish I could say there
will be no more surprises / additions, but I don't want to make
promises I cannot keep. I do not see u-boot adding support for multiple
displays anytime soon, so things should stay limited to 3 nodes (one for
hdmi, lcd and vga each) or 2 nodes on socs without hdmi.

Except on sun4i where we need 2 for hdmi for compat reasons for a total
of 4.

But my crystal ball is defunct, so I cannot promise things will stay
limited to this.

Regards,

Hans

Re: [PATCH] ARM: dts: sun4i: Add simplefb node with de_fe0-de_be0-lcd0-hdmi pipeline

[Hans de Goede]

Hi,

On 19-01-15 14:35, Hans de Goede wrote:
> Hi,
>
> On 19-01-15 14:20, Maxime Ripard wrote:
>> Hi Hans,
>>
>> On Mon, Jan 19, 2015 at 02:01:17PM +0100, Hans de Goede wrote:
>>> Testing has shown that on sun4i the display backend engine does not have
>>> deep enough fifo-s causing flickering / tearing in full-hd mode due to
>>> fifo underruns. This can be avoided by letting the display frontend engine
>>> do the dma from memory, and then letting it feed the data directly into
>>> the backend unmodified, as the frontend does have deep enough fifo-s.
>>>
>>> Note since u-boot-v2015.01 has been released using the de_be0-lcd0-hdmi
>>> pipeline on sun4i, we need to keep that one around too (unfortunately).
>>>
>>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>   arch/arm/boot/dts/sun4i-a10.dtsi | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
>>> index f5e35b5..ccd60e3 100644
>>> --- a/arch/arm/boot/dts/sun4i-a10.dtsi
>>> +++ b/arch/arm/boot/dts/sun4i-a10.dtsi
>>> @@ -44,6 +44,14 @@
>>>                    <&ahb_gates 44>;
>>>               status = "disabled";
>>>           };
>>> +
>>> +        framebuffer@1 {
>>> +            compatible = "allwinner,simple-framebuffer", "simple-framebuffer";
>>> +            allwinner,pipeline = "de_fe0-de_be0-lcd0-hdmi";
>>> +            clocks = <&pll5 1>, <&ahb_gates 36>, <&ahb_gates 43>,
>>> +                 <&ahb_gates 44>, <&ahb_gates 46>;
>>> +            status = "disabled";
>>> +        };
>>
>> Thanks for this.
>>
>> I do have a question though, maybe I'm missing something, but what
>> would prevent us from using the existing node already? The output
>> pipeline seems to be the same, at least for which output we
>> use. Wether we use or not the display frontend looks like an
>> implementation detail.
>
> The purpose of having a node per pipeline is to have the rights clocks
> listed for the pipeline chosen. So the "de_fe0-de_be0-lcd0-hdmi" node
> has one ahb_gates entry more so as to make sure the kernel does not gate
> off the frontend.
>
> The idea of having different nodes per pipeline is that when we get
> support for more module clocks in place, as a preparation for kms
> support, we can add the module clocks to the relevant nodes, to also
> ensure the module clocks will not get turned off.
>
> By having this info in the dtb, rather then in u-boot we can update
> the clocks list as we add new clock nodes in the dtb, ensuring that
> an older u-boot will keep working as we add support for module
> clocks, etc.

Hmm, thinking more about this, and about how u-boot searches for
the simplefb node, I think that having a single node like this:

         framebuffer@0 {
             compatible = "allwinner,simple-framebuffer", "simple-framebuffer";
             /* This node is for de_fe0-de_be0-lcd0-hdmi, but we also declare de_be0-lcd0-hdmi
                for compatibility with u-boot-v2015.01 which does not use the frontend */
             allwinner,pipeline = "de_fe0-de_be0-lcd0-hdmi", "de_be0-lcd0-hdmi";
             clocks = <&pll5 1>, <&ahb_gates 36>, <&ahb_gates 43>,
                  <&ahb_gates 44>, <&ahb_gates 46>;
             status = "disabled";
         };

Should work, but this will cause simplefb to enable the frontend ahb gate
even if it is not used (this will only impact u-boot-v2015.01 users), and as
such feels wrong, so I would prefer to go with my patch as is.

Regards,

Hans

Re: [PATCH] ARM: dts: sun4i: Add simplefb node with de_fe0-de_be0-lcd0-hdmi pipeline

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4152 bytes --]

On Mon, Jan 19, 2015 at 02:35:41PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 19-01-15 14:20, Maxime Ripard wrote:
> >Hi Hans,
> >
> >On Mon, Jan 19, 2015 at 02:01:17PM +0100, Hans de Goede wrote:
> >>Testing has shown that on sun4i the display backend engine does not have
> >>deep enough fifo-s causing flickering / tearing in full-hd mode due to
> >>fifo underruns. This can be avoided by letting the display frontend engine
> >>do the dma from memory, and then letting it feed the data directly into
> >>the backend unmodified, as the frontend does have deep enough fifo-s.
> >>
> >>Note since u-boot-v2015.01 has been released using the de_be0-lcd0-hdmi
> >>pipeline on sun4i, we need to keep that one around too (unfortunately).
> >>
> >>Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>---
> >>  arch/arm/boot/dts/sun4i-a10.dtsi | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >>diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
> >>index f5e35b5..ccd60e3 100644
> >>--- a/arch/arm/boot/dts/sun4i-a10.dtsi
> >>+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
> >>@@ -44,6 +44,14 @@
> >>  				 <&ahb_gates 44>;
> >>  			status = "disabled";
> >>  		};
> >>+
> >>+		framebuffer@1 {
> >>+			compatible = "allwinner,simple-framebuffer", "simple-framebuffer";
> >>+			allwinner,pipeline = "de_fe0-de_be0-lcd0-hdmi";
> >>+			clocks = <&pll5 1>, <&ahb_gates 36>, <&ahb_gates 43>,
> >>+				 <&ahb_gates 44>, <&ahb_gates 46>;
> >>+			status = "disabled";
> >>+		};
> >
> >Thanks for this.
> >
> >I do have a question though, maybe I'm missing something, but what
> >would prevent us from using the existing node already? The output
> >pipeline seems to be the same, at least for which output we
> >use. Wether we use or not the display frontend looks like an
> >implementation detail.
> 
> The purpose of having a node per pipeline is to have the rights clocks
> listed for the pipeline chosen. So the "de_fe0-de_be0-lcd0-hdmi" node
> has one ahb_gates entry more so as to make sure the kernel does not gate
> off the frontend.
> 
> The idea of having different nodes per pipeline is that when we get
> support for more module clocks in place, as a preparation for kms
> support, we can add the module clocks to the relevant nodes, to also
> ensure the module clocks will not get turned off.
> 
> By having this info in the dtb, rather then in u-boot we can update
> the clocks list as we add new clock nodes in the dtb, ensuring that
> an older u-boot will keep working as we add support for module
> clocks, etc.

Yeah, I know that, and it's exactly why we have these nodes
pre-populated it. But my point was exactly that: Why not leverage the
fact that we have that possibility to add the missing clock for the
frontend, and just use that.

> >That or just having several strings in allwinner,pipeline.
> >
> >If this simplefb binding is just going to be a collection of all the
> >various combinations we have, plus the legacy options, I'm a bit
> >worried that this is just going to be a hell to maintain, without any
> >way to remove it at compile time (if such feature is ever merged in
> >DTC).
> 
> I can understand where you're coming from, I wish I could say there
> will be no more surprises / additions, but I don't want to make
> promises I cannot keep. I do not see u-boot adding support for multiple
> displays anytime soon, so things should stay limited to 3 nodes (one for
> hdmi, lcd and vga each) or 2 nodes on socs without hdmi.
> 
> Except on sun4i where we need 2 for hdmi for compat reasons for a total
> of 4.
> 
> But my crystal ball is defunct, so I cannot promise things will stay
> limited to this.

Heh :)

I'm not really asking for a crystal ball. I even expect that list to
grow, as some board will probably need the second display engine on
SoCs that have one, and that's ok. But my concern is more that we will
end up with multiple similar options, including ones that we know are
not in use, but can't really remove either.

But yeah, ok.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] ARM: dts: sun4i: Add simplefb node with de_fe0-de_be0-lcd0-hdmi pipeline

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3935 bytes --]

On Mon, Jan 19, 2015 at 02:46:00PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 19-01-15 14:35, Hans de Goede wrote:
> >Hi,
> >
> >On 19-01-15 14:20, Maxime Ripard wrote:
> >>Hi Hans,
> >>
> >>On Mon, Jan 19, 2015 at 02:01:17PM +0100, Hans de Goede wrote:
> >>>Testing has shown that on sun4i the display backend engine does not have
> >>>deep enough fifo-s causing flickering / tearing in full-hd mode due to
> >>>fifo underruns. This can be avoided by letting the display frontend engine
> >>>do the dma from memory, and then letting it feed the data directly into
> >>>the backend unmodified, as the frontend does have deep enough fifo-s.
> >>>
> >>>Note since u-boot-v2015.01 has been released using the de_be0-lcd0-hdmi
> >>>pipeline on sun4i, we need to keep that one around too (unfortunately).
> >>>
> >>>Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>>---
> >>>  arch/arm/boot/dts/sun4i-a10.dtsi | 8 ++++++++
> >>>  1 file changed, 8 insertions(+)
> >>>
> >>>diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
> >>>index f5e35b5..ccd60e3 100644
> >>>--- a/arch/arm/boot/dts/sun4i-a10.dtsi
> >>>+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
> >>>@@ -44,6 +44,14 @@
> >>>                   <&ahb_gates 44>;
> >>>              status = "disabled";
> >>>          };
> >>>+
> >>>+        framebuffer@1 {
> >>>+            compatible = "allwinner,simple-framebuffer", "simple-framebuffer";
> >>>+            allwinner,pipeline = "de_fe0-de_be0-lcd0-hdmi";
> >>>+            clocks = <&pll5 1>, <&ahb_gates 36>, <&ahb_gates 43>,
> >>>+                 <&ahb_gates 44>, <&ahb_gates 46>;
> >>>+            status = "disabled";
> >>>+        };
> >>
> >>Thanks for this.
> >>
> >>I do have a question though, maybe I'm missing something, but what
> >>would prevent us from using the existing node already? The output
> >>pipeline seems to be the same, at least for which output we
> >>use. Wether we use or not the display frontend looks like an
> >>implementation detail.
> >
> >The purpose of having a node per pipeline is to have the rights clocks
> >listed for the pipeline chosen. So the "de_fe0-de_be0-lcd0-hdmi" node
> >has one ahb_gates entry more so as to make sure the kernel does not gate
> >off the frontend.
> >
> >The idea of having different nodes per pipeline is that when we get
> >support for more module clocks in place, as a preparation for kms
> >support, we can add the module clocks to the relevant nodes, to also
> >ensure the module clocks will not get turned off.
> >
> >By having this info in the dtb, rather then in u-boot we can update
> >the clocks list as we add new clock nodes in the dtb, ensuring that
> >an older u-boot will keep working as we add support for module
> >clocks, etc.
> 
> Hmm, thinking more about this, and about how u-boot searches for
> the simplefb node, I think that having a single node like this:
> 
>         framebuffer@0 {
>             compatible = "allwinner,simple-framebuffer", "simple-framebuffer";
>             /* This node is for de_fe0-de_be0-lcd0-hdmi, but we also declare de_be0-lcd0-hdmi
>                for compatibility with u-boot-v2015.01 which does not use the frontend */
>             allwinner,pipeline = "de_fe0-de_be0-lcd0-hdmi", "de_be0-lcd0-hdmi";
>             clocks = <&pll5 1>, <&ahb_gates 36>, <&ahb_gates 43>,
>                  <&ahb_gates 44>, <&ahb_gates 46>;
>             status = "disabled";
>         };
> 
> Should work, but this will cause simplefb to enable the frontend ahb gate
> even if it is not used (this will only impact u-boot-v2015.01 users), and as
> such feels wrong, so I would prefer to go with my patch as is.

That's actually what I had in mind. It didn't feel that wrong, since
it pretty much mimics the compatible behaviour, but it's your call.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] ARM: dts: sun4i: Add simplefb node with de_fe0-de_be0-lcd0-hdmi pipeline

[Hans de Goede]

Hi,

On 19-01-15 22:32, Maxime Ripard wrote:
> On Mon, Jan 19, 2015 at 02:46:00PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 19-01-15 14:35, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 19-01-15 14:20, Maxime Ripard wrote:
>>>> Hi Hans,
>>>>
>>>> On Mon, Jan 19, 2015 at 02:01:17PM +0100, Hans de Goede wrote:
>>>>> Testing has shown that on sun4i the display backend engine does not have
>>>>> deep enough fifo-s causing flickering / tearing in full-hd mode due to
>>>>> fifo underruns. This can be avoided by letting the display frontend engine
>>>>> do the dma from memory, and then letting it feed the data directly into
>>>>> the backend unmodified, as the frontend does have deep enough fifo-s.
>>>>>
>>>>> Note since u-boot-v2015.01 has been released using the de_be0-lcd0-hdmi
>>>>> pipeline on sun4i, we need to keep that one around too (unfortunately).
>>>>>
>>>>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>>> ---
>>>>>   arch/arm/boot/dts/sun4i-a10.dtsi | 8 ++++++++
>>>>>   1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
>>>>> index f5e35b5..ccd60e3 100644
>>>>> --- a/arch/arm/boot/dts/sun4i-a10.dtsi
>>>>> +++ b/arch/arm/boot/dts/sun4i-a10.dtsi
>>>>> @@ -44,6 +44,14 @@
>>>>>                    <&ahb_gates 44>;
>>>>>               status = "disabled";
>>>>>           };
>>>>> +
>>>>> +        framebuffer@1 {
>>>>> +            compatible = "allwinner,simple-framebuffer", "simple-framebuffer";
>>>>> +            allwinner,pipeline = "de_fe0-de_be0-lcd0-hdmi";
>>>>> +            clocks = <&pll5 1>, <&ahb_gates 36>, <&ahb_gates 43>,
>>>>> +                 <&ahb_gates 44>, <&ahb_gates 46>;
>>>>> +            status = "disabled";
>>>>> +        };
>>>>
>>>> Thanks for this.
>>>>
>>>> I do have a question though, maybe I'm missing something, but what
>>>> would prevent us from using the existing node already? The output
>>>> pipeline seems to be the same, at least for which output we
>>>> use. Wether we use or not the display frontend looks like an
>>>> implementation detail.
>>>
>>> The purpose of having a node per pipeline is to have the rights clocks
>>> listed for the pipeline chosen. So the "de_fe0-de_be0-lcd0-hdmi" node
>>> has one ahb_gates entry more so as to make sure the kernel does not gate
>>> off the frontend.
>>>
>>> The idea of having different nodes per pipeline is that when we get
>>> support for more module clocks in place, as a preparation for kms
>>> support, we can add the module clocks to the relevant nodes, to also
>>> ensure the module clocks will not get turned off.
>>>
>>> By having this info in the dtb, rather then in u-boot we can update
>>> the clocks list as we add new clock nodes in the dtb, ensuring that
>>> an older u-boot will keep working as we add support for module
>>> clocks, etc.
>>
>> Hmm, thinking more about this, and about how u-boot searches for
>> the simplefb node, I think that having a single node like this:
>>
>>          framebuffer@0 {
>>              compatible = "allwinner,simple-framebuffer", "simple-framebuffer";
>>              /* This node is for de_fe0-de_be0-lcd0-hdmi, but we also declare de_be0-lcd0-hdmi
>>                 for compatibility with u-boot-v2015.01 which does not use the frontend */
>>              allwinner,pipeline = "de_fe0-de_be0-lcd0-hdmi", "de_be0-lcd0-hdmi";
>>              clocks = <&pll5 1>, <&ahb_gates 36>, <&ahb_gates 43>,
>>                   <&ahb_gates 44>, <&ahb_gates 46>;
>>              status = "disabled";
>>          };
>>
>> Should work, but this will cause simplefb to enable the frontend ahb gate
>> even if it is not used (this will only impact u-boot-v2015.01 users), and as
>> such feels wrong, so I would prefer to go with my patch as is.
>
> That's actually what I had in mind. It didn't feel that wrong, since
> it pretty much mimics the compatible behaviour, but it's your call.

If it is my call then I would prefer to go with the 2 separate nodes + v2
of my lcd / vga nodes addition patch. If things indeed do become the combinatorial
explosion you fear in the future, then we can always move to listing multiple
pipelines in one node then.

Thanks & Regards,

Hans

Re: [PATCH] ARM: dts: sun4i: Add simplefb node with de_fe0-de_be0-lcd0-hdmi pipeline

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 802 bytes --]

On Mon, Jan 19, 2015 at 02:01:17PM +0100, Hans de Goede wrote:
> Testing has shown that on sun4i the display backend engine does not have
> deep enough fifo-s causing flickering / tearing in full-hd mode due to
> fifo underruns. This can be avoided by letting the display frontend engine
> do the dma from memory, and then letting it feed the data directly into
> the backend unmodified, as the frontend does have deep enough fifo-s.
> 
> Note since u-boot-v2015.01 has been released using the de_be0-lcd0-hdmi
> pipeline on sun4i, we need to keep that one around too (unfortunately).
> 
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Merged, thanks!

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com