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

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

Signed-off-by Hans de Goede <[email protected]>

---
 arch/arm/boot/dts/sun4i-a10.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

arch/arm/boot/dts/sun4i-a10.dtsi +0 -0

index f5e35b5..ccd60e3 100644
@@ -44,6 +44,14 @@
 				 <&ahb_gates 44>;

Hi Hans,

Quoted 20 lines · 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 <[email protected]>

---
  arch/arm/boot/dts/sun4i-a10.dtsi | 8 ++++++++
  1 file changed, 8 insertions(+)

arch/arm/boot/dts/sun4i-a10.dtsi +0 -0

index f5e35b5..ccd60e3 100644
@@ -44,6 +44,14 @@
  				 <&ahb_gates 44>;

arch/arm/boot/dts/sun4i-a10.dtsi line 54

Quoted 10 lines

arch/arm/boot/dts/sun4i-a10.dtsi +8 -0

  			status = "disabled";
  		};
+
+		framebuffer at 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).

Hi,

Quoted 36 lines · Maxime Ripard

Hi Hans,

Quoted 20 lines · 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 <[email protected]>

---
  arch/arm/boot/dts/sun4i-a10.dtsi | 8 ++++++++
  1 file changed, 8 insertions(+)

arch/arm/boot/dts/sun4i-a10.dtsi +0 -0

index f5e35b5..ccd60e3 100644
@@ -44,6 +44,14 @@
                   <&ahb_gates 44>;

arch/arm/boot/dts/sun4i-a10.dtsi line 54

Quoted 10 lines

arch/arm/boot/dts/sun4i-a10.dtsi +8 -0

              status = "disabled";
          };
+
+        framebuffer at 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.

Quoted 5 lines

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.

Hi,

Quoted 55 lines · Hans de Goede

Hi,

Quoted 36 lines · Maxime Ripard

Hi Hans,

Quoted 20 lines · 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 <[email protected]>

---
 arch/arm/boot/dts/sun4i-a10.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

arch/arm/boot/dts/sun4i-a10.dtsi +0 -0

index f5e35b5..ccd60e3 100644
@@ -44,6 +44,14 @@
                  <&ahb_gates 44>;

arch/arm/boot/dts/sun4i-a10.dtsi line 54

Quoted 10 lines

arch/arm/boot/dts/sun4i-a10.dtsi +8 -0

             status = "disabled";
         };
+
+        framebuffer at 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.

Quoted 5 lines

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.

Quoted 4 lines

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

Quoted 76 lines · Hans de Goede

Hi,

Quoted 55 lines · Hans de Goede

Hi,

Quoted 36 lines · Maxime Ripard

Hi Hans,

Quoted 20 lines · 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 <[email protected]>

---
  arch/arm/boot/dts/sun4i-a10.dtsi | 8 ++++++++
  1 file changed, 8 insertions(+)

arch/arm/boot/dts/sun4i-a10.dtsi +0 -0

index f5e35b5..ccd60e3 100644
@@ -44,6 +44,14 @@
                   <&ahb_gates 44>;

arch/arm/boot/dts/sun4i-a10.dtsi line 54

Quoted 10 lines

arch/arm/boot/dts/sun4i-a10.dtsi +8 -0

              status = "disabled";
          };
+
+        framebuffer at 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.

Quoted 5 lines

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.

Quoted 4 lines

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 at 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";
         };

Quoted 3 lines

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.

Hi,

Quoted 36 lines · Maxime Ripard

Hi Hans,

Quoted 20 lines · 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 <[email protected]>

---
 arch/arm/boot/dts/sun4i-a10.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

arch/arm/boot/dts/sun4i-a10.dtsi +0 -0

index f5e35b5..ccd60e3 100644
@@ -44,6 +44,14 @@
 				 <&ahb_gates 44>;

arch/arm/boot/dts/sun4i-a10.dtsi line 54

Quoted 10 lines

arch/arm/boot/dts/sun4i-a10.dtsi +8 -0

 			status = "disabled";
 		};
+
+		framebuffer at 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.

Quoted 5 lines

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.

Quoted 7 lines

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.

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 <[email protected]>