[PATCH 0/7] drm/sun4i: update DE33 support

[PATCH 0/7] drm/sun4i: update DE33 support

[Jernej Skrabec]

This is second series out of tree which aims at properly introducing
support for H616 Display Engine 3.3. Previous series [1] reorganized
driver so proper DE33 support can be easily implemented.

H616 DE33 support was actually introduced a while back, but it was done
without fully understanding hardware design. Fortunately, no user of
H616 DE33 binding was introduced, so we have a chance to update bindings
and introduce proper DE33 support. Issue with existing binding is that it
considers planes as resource which is hardwired to each mixer as it was
done on older Display Engine generations (DE3 and lower). That is not the
case anymore. This series introduces new driver for planes management,
which allows doing proper plane assignments.

Remaining patches, which introduce all the missing bits to fully support
display pipeline on H616 SoC, will be sent once this series is merged.
WIP patches, which can be used for testing purposes, can be found at [2].

Please take a look.

Best regards,
Jernej

[1] https://lore.kernel.org/linux-sunxi/20251104180942.61538-1-jernej.skrabec@gmail.com/T/#t
[2] https://github.com/jernejsk/linux-1/commits/sun4i-drm-refactor/

Jernej Skrabec (7):
  drm/sun4i: Add support for DE33 CSC
  drm/sun4i: vi_layer: Limit formats for DE33
  clk: sunxi-ng: de2: Export register regmap for DE33
  dt-bindings: display: allwinner: Add DE33 planes
  drm/sun4i: Add planes driver
  dt-bindings: display: allwinner: Update H616 DE33 binding
  drm/sun4i: switch DE33 to new bindings

 .../allwinner,sun50i-h616-de33-planes.yaml    |  44 ++++
 .../allwinner,sun8i-a83t-de2-mixer.yaml       |  16 +-
 drivers/clk/sunxi-ng/ccu-sun8i-de2.c          |  53 ++++-
 drivers/gpu/drm/sun4i/Kconfig                 |   8 +
 drivers/gpu/drm/sun4i/Makefile                |   1 +
 drivers/gpu/drm/sun4i/sun50i_planes.c         | 205 ++++++++++++++++++
 drivers/gpu/drm/sun4i/sun50i_planes.h         |  43 ++++
 drivers/gpu/drm/sun4i/sun8i_csc.c             |  71 ++++++
 drivers/gpu/drm/sun4i/sun8i_csc.h             |   5 +
 drivers/gpu/drm/sun4i/sun8i_mixer.c           | 130 ++++++-----
 drivers/gpu/drm/sun4i/sun8i_mixer.h           |  10 +-
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c        |  36 ++-
 12 files changed, 543 insertions(+), 79 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml
 create mode 100644 drivers/gpu/drm/sun4i/sun50i_planes.c
 create mode 100644 drivers/gpu/drm/sun4i/sun50i_planes.h

-- 
2.51.2

[PATCH 1/7] drm/sun4i: Add support for DE33 CSC

[Jernej Skrabec]

DE33 has channel CSC units (for each plane separately) so pipeline can
be configured to output in desired colorspace.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/gpu/drm/sun4i/sun8i_csc.c | 71 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/sun4i/sun8i_csc.h |  5 +++
 2 files changed, 76 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun8i_csc.c b/drivers/gpu/drm/sun4i/sun8i_csc.c
index ce81c12f511d..70fc9b017d17 100644
--- a/drivers/gpu/drm/sun4i/sun8i_csc.c
+++ b/drivers/gpu/drm/sun4i/sun8i_csc.c
@@ -205,6 +205,72 @@ static void sun8i_de3_ccsc_setup(struct regmap *map, int layer,
 			   mask, val);
 }
 
+/* extract constant from high word and invert sign if necessary */
+static u32 sun8i_de33_ccsc_get_constant(u32 value)
+{
+	value >>= 16;
+
+	if (value & BIT(15))
+		return 0x400 - (value & 0x3ff);
+
+	return value;
+}
+
+static void sun8i_de33_convert_table(const u32 *src, u32 *dst)
+{
+	dst[0] = sun8i_de33_ccsc_get_constant(src[3]);
+	dst[1] = sun8i_de33_ccsc_get_constant(src[7]);
+	dst[2] = sun8i_de33_ccsc_get_constant(src[11]);
+	memcpy(&dst[3], src, sizeof(u32) * 12);
+	dst[6] &= 0xffff;
+	dst[10] &= 0xffff;
+	dst[14] &= 0xffff;
+}
+
+static void sun8i_de33_ccsc_setup(struct regmap *map, int layer,
+				  enum sun8i_csc_mode mode,
+				  enum drm_color_encoding encoding,
+				  enum drm_color_range range)
+{
+	u32 addr, val, base, csc[15];
+	const u32 *table;
+	int i;
+
+	table = yuv2rgb_de3[range][encoding];
+	base = DE33_CCSC_BASE + layer * DE33_CH_SIZE;
+
+	switch (mode) {
+	case SUN8I_CSC_MODE_OFF:
+		val = 0;
+		break;
+	case SUN8I_CSC_MODE_YUV2RGB:
+		val = SUN8I_CSC_CTRL_EN;
+		sun8i_de33_convert_table(table, csc);
+		regmap_bulk_write(map, SUN50I_CSC_COEFF(base, 0), csc, 15);
+		break;
+	case SUN8I_CSC_MODE_YVU2RGB:
+		val = SUN8I_CSC_CTRL_EN;
+		sun8i_de33_convert_table(table, csc);
+		for (i = 0; i < 15; i++) {
+			addr = SUN50I_CSC_COEFF(base, i);
+			if (i > 3) {
+				if (((i - 3) & 3) == 1)
+					addr = SUN50I_CSC_COEFF(base, i + 1);
+				else if (((i - 3) & 3) == 2)
+					addr = SUN50I_CSC_COEFF(base, i - 1);
+			}
+			regmap_write(map, addr, csc[i]);
+		}
+		break;
+	default:
+		val = 0;
+		DRM_WARN("Wrong CSC mode specified.\n");
+		return;
+	}
+
+	regmap_write(map, SUN8I_CSC_CTRL(base), val);
+}
+
 static u32 sun8i_csc_get_mode(struct drm_plane_state *state)
 {
 	const struct drm_format_info *format;
@@ -238,6 +304,11 @@ void sun8i_csc_config(struct sun8i_layer *layer,
 				     mode, state->color_encoding,
 				     state->color_range);
 		return;
+	} else if (layer->cfg->de_type == SUN8I_MIXER_DE33) {
+		sun8i_de33_ccsc_setup(layer->regs, layer->channel,
+				      mode, state->color_encoding,
+				      state->color_range);
+		return;
 	}
 
 	base = ccsc_base[layer->cfg->ccsc][layer->channel];
diff --git a/drivers/gpu/drm/sun4i/sun8i_csc.h b/drivers/gpu/drm/sun4i/sun8i_csc.h
index 2a4b79599610..d2ba5f8611aa 100644
--- a/drivers/gpu/drm/sun4i/sun8i_csc.h
+++ b/drivers/gpu/drm/sun4i/sun8i_csc.h
@@ -18,9 +18,14 @@ struct sun8i_layer;
 #define CCSC10_OFFSET 0xA0000
 #define CCSC11_OFFSET 0xF0000
 
+#define DE33_CCSC_BASE 0x800
+
 #define SUN8I_CSC_CTRL(base)		((base) + 0x0)
 #define SUN8I_CSC_COEFF(base, i)	((base) + 0x10 + 4 * (i))
 
+#define SUN50I_CSC_COEFF(base, i)	((base) + 0x04 + 4 * (i))
+#define SUN50I_CSC_ALPHA(base)		((base) + 0x40)
+
 #define SUN8I_CSC_CTRL_EN		BIT(0)
 
 void sun8i_csc_config(struct sun8i_layer *layer,
-- 
2.51.2

Re: [PATCH 1/7] drm/sun4i: Add support for DE33 CSC

[Chen-Yu Tsai]

On Sat, Nov 15, 2025 at 10:14 PM Jernej Skrabec
<jernej.skrabec@gmail.com> wrote:
>
> DE33 has channel CSC units (for each plane separately) so pipeline can
> be configured to output in desired colorspace.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Reviewed-by: Chen-Yu Tsai <wens@kernel.org>

[PATCH 2/7] drm/sun4i: vi_layer: Limit formats for DE33

[Jernej Skrabec]

YUV formats need scaler support due to chroma upscaling, but that's not
yet supported in the driver. Remove them from supported list until
DE33 scaler is properly supported.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 36 +++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
index 40008c38003d..baa240c4bb82 100644
--- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
@@ -403,6 +403,37 @@ static const u32 sun8i_vi_layer_de3_formats[] = {
 	DRM_FORMAT_YVU422,
 };
 
+/*
+ * TODO: DE33 VI planes naturally support YUV formats but
+ * driver needs improvements in order to support them.
+ */
+static const u32 sun8i_vi_layer_de33_formats[] = {
+	DRM_FORMAT_ABGR1555,
+	DRM_FORMAT_ABGR2101010,
+	DRM_FORMAT_ABGR4444,
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_ARGB1555,
+	DRM_FORMAT_ARGB2101010,
+	DRM_FORMAT_ARGB4444,
+	DRM_FORMAT_ARGB8888,
+	DRM_FORMAT_BGR565,
+	DRM_FORMAT_BGR888,
+	DRM_FORMAT_BGRA1010102,
+	DRM_FORMAT_BGRA5551,
+	DRM_FORMAT_BGRA4444,
+	DRM_FORMAT_BGRA8888,
+	DRM_FORMAT_BGRX8888,
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_RGB888,
+	DRM_FORMAT_RGBA1010102,
+	DRM_FORMAT_RGBA4444,
+	DRM_FORMAT_RGBA5551,
+	DRM_FORMAT_RGBA8888,
+	DRM_FORMAT_RGBX8888,
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_XRGB8888,
+};
+
 static const uint64_t sun8i_layer_modifiers[] = {
 	DRM_FORMAT_MOD_LINEAR,
 	DRM_FORMAT_MOD_INVALID
@@ -432,7 +463,10 @@ struct sun8i_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
 	layer->regs = regs;
 	layer->cfg = cfg;
 
-	if (layer->cfg->de_type >= SUN8I_MIXER_DE3) {
+	if (layer->cfg->de_type == SUN8I_MIXER_DE33) {
+		formats = sun8i_vi_layer_de33_formats;
+		format_count = ARRAY_SIZE(sun8i_vi_layer_de33_formats);
+	} else if (layer->cfg->de_type == SUN8I_MIXER_DE3) {
 		formats = sun8i_vi_layer_de3_formats;
 		format_count = ARRAY_SIZE(sun8i_vi_layer_de3_formats);
 	} else {
-- 
2.51.2

Re: [PATCH 2/7] drm/sun4i: vi_layer: Limit formats for DE33

[Chen-Yu Tsai]

On Sat, Nov 15, 2025 at 10:14 PM Jernej Skrabec
<jernej.skrabec@gmail.com> wrote:
>
> YUV formats need scaler support due to chroma upscaling, but that's not
> yet supported in the driver. Remove them from supported list until
> DE33 scaler is properly supported.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Reviewed-by: Chen-Yu Tsai <wens@kernel.org>

I assume a fixes tag isn't needed because technically DE33 support isn't
there yet?

Re: [PATCH 2/7] drm/sun4i: vi_layer: Limit formats for DE33

[Jernej Škrabec]

Dne sobota, 15. november 2025 ob 15:40:27 Srednjeevropski standardni čas je Chen-Yu Tsai napisal(a):
> On Sat, Nov 15, 2025 at 10:14 PM Jernej Skrabec
> <jernej.skrabec@gmail.com> wrote:
> >
> > YUV formats need scaler support due to chroma upscaling, but that's not
> > yet supported in the driver. Remove them from supported list until
> > DE33 scaler is properly supported.
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> 
> Reviewed-by: Chen-Yu Tsai <wens@kernel.org>
> 
> I assume a fixes tag isn't needed because technically DE33 support isn't
> there yet?
> 

There is no user of DE33 bindings yet, so yes.

Best regards
Jernej

[PATCH 3/7] clk: sunxi-ng: de2: Export register regmap for DE33

[Jernej Skrabec]

DE33 clock pre-set plane mapping, which is not something that we want
from clock driver. Export registers instead, so DRM driver can set them
properly.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/clk/sunxi-ng/ccu-sun8i-de2.c | 53 ++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c
index a6cd0f988859..2841ec922025 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c
@@ -6,9 +6,11 @@
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/io.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 #include <linux/reset.h>
 
 #include "ccu_common.h"
@@ -250,6 +252,41 @@ static const struct sunxi_ccu_desc sun50i_h616_de33_clk_desc = {
 	.num_resets	= ARRAY_SIZE(sun50i_h5_de2_resets),
 };
 
+/*
+ * Add a regmap for the DE33 plane driver to access plane
+ * mapping registers.
+ * Only these registers are allowed to be written, to prevent
+ * overriding clock and reset configuration.
+ */
+
+#define SUN50I_DE33_CHN2CORE_REG 0x24
+#define SUN50I_DE33_PORT02CHN_REG 0x28
+#define SUN50I_DE33_PORT12CHN_REG 0x2c
+
+static bool sun8i_de2_ccu_regmap_accessible_reg(struct device *dev,
+						unsigned int reg)
+{
+	switch (reg) {
+	case SUN50I_DE33_CHN2CORE_REG:
+	case SUN50I_DE33_PORT02CHN_REG:
+	case SUN50I_DE33_PORT12CHN_REG:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config sun8i_de2_ccu_regmap_config = {
+	.reg_bits	= 32,
+	.val_bits	= 32,
+	.reg_stride	= 4,
+	.max_register	= 0xe0,
+
+	/* other devices have no business accessing other registers */
+	.readable_reg	= sun8i_de2_ccu_regmap_accessible_reg,
+	.writeable_reg	= sun8i_de2_ccu_regmap_accessible_reg,
+};
+
 static int sunxi_de2_clk_probe(struct platform_device *pdev)
 {
 	struct clk *bus_clk, *mod_clk;
@@ -303,13 +340,23 @@ static int sunxi_de2_clk_probe(struct platform_device *pdev)
 	}
 
 	/*
-	 * The DE33 requires these additional (unknown) registers set
+	 * The DE33 requires these additional plane mapping registers set
 	 * during initialisation.
 	 */
 	if (of_device_is_compatible(pdev->dev.of_node,
 				    "allwinner,sun50i-h616-de33-clk")) {
-		writel(0, reg + 0x24);
-		writel(0x0000a980, reg + 0x28);
+		struct regmap *regmap;
+
+		regmap = devm_regmap_init_mmio(&pdev->dev, reg,
+					       &sun8i_de2_ccu_regmap_config);
+		if (IS_ERR(regmap)) {
+			ret = PTR_ERR(regmap);
+			goto err_assert_reset;
+		}
+
+		ret = of_syscon_register_regmap(pdev->dev.of_node, regmap);
+		if (ret)
+			goto err_assert_reset;
 	}
 
 	ret = devm_sunxi_ccu_probe(&pdev->dev, reg, ccu_desc);
-- 
2.51.2

Re: [PATCH 3/7] clk: sunxi-ng: de2: Export register regmap for DE33

[Chen-Yu Tsai]

On Sat, Nov 15, 2025 at 10:14 PM Jernej Skrabec
<jernej.skrabec@gmail.com> wrote:
>
> DE33 clock pre-set plane mapping, which is not something that we want
> from clock driver. Export registers instead, so DRM driver can set them
> properly.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/clk/sunxi-ng/ccu-sun8i-de2.c | 53 ++++++++++++++++++++++++++--
>  1 file changed, 50 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c
> index a6cd0f988859..2841ec922025 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c
> @@ -6,9 +6,11 @@
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/io.h>
> +#include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/regmap.h>
>  #include <linux/reset.h>
>
>  #include "ccu_common.h"
> @@ -250,6 +252,41 @@ static const struct sunxi_ccu_desc sun50i_h616_de33_clk_desc = {
>         .num_resets     = ARRAY_SIZE(sun50i_h5_de2_resets),
>  };
>
> +/*
> + * Add a regmap for the DE33 plane driver to access plane
> + * mapping registers.
> + * Only these registers are allowed to be written, to prevent
> + * overriding clock and reset configuration.
> + */
> +
> +#define SUN50I_DE33_CHN2CORE_REG 0x24
> +#define SUN50I_DE33_PORT02CHN_REG 0x28
> +#define SUN50I_DE33_PORT12CHN_REG 0x2c
> +
> +static bool sun8i_de2_ccu_regmap_accessible_reg(struct device *dev,
> +                                               unsigned int reg)
> +{
> +       switch (reg) {
> +       case SUN50I_DE33_CHN2CORE_REG:
> +       case SUN50I_DE33_PORT02CHN_REG:
> +       case SUN50I_DE33_PORT12CHN_REG:
> +               return true;
> +       default:
> +               return false;
> +       }
> +}

Since the registers are contiguous, I think it makes a bit more sense
to use the .rd_table and .wr_table. A bonus is that the check can be
inlined in the core instead of calling a function pointer.

> +static const struct regmap_config sun8i_de2_ccu_regmap_config = {
> +       .reg_bits       = 32,
> +       .val_bits       = 32,
> +       .reg_stride     = 4,
> +       .max_register   = 0xe0,

None of the registers past SUN50I_DE33_PORT12CHN_REG are accessible,
so we should probably just put that here.

> +
> +       /* other devices have no business accessing other registers */
> +       .readable_reg   = sun8i_de2_ccu_regmap_accessible_reg,
> +       .writeable_reg  = sun8i_de2_ccu_regmap_accessible_reg,
> +};
> +
>  static int sunxi_de2_clk_probe(struct platform_device *pdev)
>  {
>         struct clk *bus_clk, *mod_clk;
> @@ -303,13 +340,23 @@ static int sunxi_de2_clk_probe(struct platform_device *pdev)
>         }
>
>         /*
> -        * The DE33 requires these additional (unknown) registers set
> +        * The DE33 requires these additional plane mapping registers set
>          * during initialisation.
>          */
>         if (of_device_is_compatible(pdev->dev.of_node,
>                                     "allwinner,sun50i-h616-de33-clk")) {
> -               writel(0, reg + 0x24);
> -               writel(0x0000a980, reg + 0x28);
> +               struct regmap *regmap;
> +
> +               regmap = devm_regmap_init_mmio(&pdev->dev, reg,
> +                                              &sun8i_de2_ccu_regmap_config);
> +               if (IS_ERR(regmap)) {
> +                       ret = PTR_ERR(regmap);
> +                       goto err_assert_reset;
> +               }
> +
> +               ret = of_syscon_register_regmap(pdev->dev.of_node, regmap);

dev_of_node(&pdev->dev) instead of directly accessing the member.
IIRC this is the new preferred style.


Thanks
ChenYu

> +               if (ret)
> +                       goto err_assert_reset;
>         }
>
>         ret = devm_sunxi_ccu_probe(&pdev->dev, reg, ccu_desc);
> --
> 2.51.2
>
>

[PATCH 4/7] dt-bindings: display: allwinner: Add DE33 planes

[Jernej Skrabec]

Allwinner Display Engine 3.3 contains planes, which are shared resources
between all mixers present in SoC. They can be assigned to specific
mixer by using registers which reside in display clocks MMIO.

Add a binding for them.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 .../allwinner,sun50i-h616-de33-planes.yaml    | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml

diff --git a/Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml b/Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml
new file mode 100644
index 000000000000..801e5068a6b5
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/allwinner,sun50i-h616-de33-planes.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Allwinner H616 Display Engine 3.3 planes
+
+maintainers:
+  - Jernej Skrabec <jernej.skrabec@gmail.com>
+
+description: |
+  Display Engine 3.3 planes are independent of mixers, contrary to
+  previous generations of Display Engine. Planes can be assigned to
+  mixers independently and even dynamically during runtime.
+
+properties:
+  compatible:
+    enum:
+      - allwinner,sun50i-h616-de33-planes
+
+  reg:
+    maxItems: 1
+
+  allwinner,plane-mapping:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: Phandle of Display Engine clock node
+
+required:
+  - compatible
+  - reg
+  - allwinner,plane-mapping
+
+additionalProperties: false
+
+examples:
+  - |
+    planes: planes@100000 {
+        compatible = "allwinner,sun50i-h616-de33-planes";
+        reg = <0x100000 0x180000>;
+        allwinner,plane-mapping = <&display_clocks>;
+    };
+
+...
-- 
2.51.2

Re: [PATCH 4/7] dt-bindings: display: allwinner: Add DE33 planes

[Krzysztof Kozlowski]

On Sat, Nov 15, 2025 at 03:13:44PM +0100, Jernej Skrabec wrote:
> Allwinner Display Engine 3.3 contains planes, which are shared resources
> between all mixers present in SoC. They can be assigned to specific
> mixer by using registers which reside in display clocks MMIO.
> 
> Add a binding for them.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  .../allwinner,sun50i-h616-de33-planes.yaml    | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml b/Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml
> new file mode 100644
> index 000000000000..801e5068a6b5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/allwinner,sun50i-h616-de33-planes.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Allwinner H616 Display Engine 3.3 planes
> +
> +maintainers:
> +  - Jernej Skrabec <jernej.skrabec@gmail.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  Display Engine 3.3 planes are independent of mixers, contrary to
> +  previous generations of Display Engine. Planes can be assigned to
> +  mixers independently and even dynamically during runtime.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - allwinner,sun50i-h616-de33-planes
> +
> +  reg:
> +    maxItems: 1
> +
> +  allwinner,plane-mapping:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Phandle of Display Engine clock node

You description is almost duplicating property name. You need to explain
here how this device uses them.

Esxpecially that clocks do not go via custom properties.

Best regards,
Krzysztof

Re: [PATCH 4/7] dt-bindings: display: allwinner: Add DE33 planes

[Jernej Škrabec]

Hi!

Dne nedelja, 16. november 2025 ob 12:29:27 Srednjeevropski standardni čas je Krzysztof Kozlowski napisal(a):
> On Sat, Nov 15, 2025 at 03:13:44PM +0100, Jernej Skrabec wrote:
> > Allwinner Display Engine 3.3 contains planes, which are shared resources
> > between all mixers present in SoC. They can be assigned to specific
> > mixer by using registers which reside in display clocks MMIO.
> > 
> > Add a binding for them.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > ---
> >  .../allwinner,sun50i-h616-de33-planes.yaml    | 44 +++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml b/Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml
> > new file mode 100644
> > index 000000000000..801e5068a6b5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml
> > @@ -0,0 +1,44 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/allwinner,sun50i-h616-de33-planes.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Allwinner H616 Display Engine 3.3 planes
> > +
> > +maintainers:
> > +  - Jernej Skrabec <jernej.skrabec@gmail.com>
> > +
> > +description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 
> > +  Display Engine 3.3 planes are independent of mixers, contrary to
> > +  previous generations of Display Engine. Planes can be assigned to
> > +  mixers independently and even dynamically during runtime.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - allwinner,sun50i-h616-de33-planes
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  allwinner,plane-mapping:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: Phandle of Display Engine clock node
> 
> You description is almost duplicating property name. You need to explain
> here how this device uses them.

So I guess I can copy commit description here? It is needed to
access registers from different core, so it can assign (map)
planes between mixers at runtime.

> 
> Esxpecially that clocks do not go via custom properties.

This has nothing to do with clocks per se, it's just that registers
that driver needs to access for mapping planes between mixers
are in IP core which takes care mostly for clocks and resets.

Best regards,
Jernej

> 
> Best regards,
> Krzysztof
> 
> 

Re: [PATCH 4/7] dt-bindings: display: allwinner: Add DE33 planes

[Krzysztof Kozlowski]

On 16/11/2025 12:44, Jernej Škrabec wrote:
> Hi!
> 
> Dne nedelja, 16. november 2025 ob 12:29:27 Srednjeevropski standardni čas je Krzysztof Kozlowski napisal(a):
>> On Sat, Nov 15, 2025 at 03:13:44PM +0100, Jernej Skrabec wrote:
>>> Allwinner Display Engine 3.3 contains planes, which are shared resources
>>> between all mixers present in SoC. They can be assigned to specific
>>> mixer by using registers which reside in display clocks MMIO.
>>>
>>> Add a binding for them.
>>>
>>> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
>>> ---
>>>  .../allwinner,sun50i-h616-de33-planes.yaml    | 44 +++++++++++++++++++
>>>  1 file changed, 44 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml b/Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml
>>> new file mode 100644
>>> index 000000000000..801e5068a6b5
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml
>>> @@ -0,0 +1,44 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/display/allwinner,sun50i-h616-de33-planes.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Allwinner H616 Display Engine 3.3 planes
>>> +
>>> +maintainers:
>>> +  - Jernej Skrabec <jernej.skrabec@gmail.com>
>>> +
>>> +description: |
>>
>> Do not need '|' unless you need to preserve formatting.
>>
>>> +  Display Engine 3.3 planes are independent of mixers, contrary to
>>> +  previous generations of Display Engine. Planes can be assigned to
>>> +  mixers independently and even dynamically during runtime.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - allwinner,sun50i-h616-de33-planes
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  allwinner,plane-mapping:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description: Phandle of Display Engine clock node
>>
>> You description is almost duplicating property name. You need to explain
>> here how this device uses them.
> 
> So I guess I can copy commit description here? It is needed to
> access registers from different core, so it can assign (map)
> planes between mixers at runtime.


"to assign (map) planes between mixers." is enough.

But it looks unfortunately like a spaghetti.

Your mixer binding references via phandle this planes. These planes
reference via phandle some other region to configure planes between mixers.

Isn't this the job of this device?


Best regards,
Krzysztof

Re: [PATCH 4/7] dt-bindings: display: allwinner: Add DE33 planes

[Jernej Škrabec]

Dne nedelja, 16. november 2025 ob 12:49:45 Srednjeevropski standardni čas je Krzysztof Kozlowski napisal(a):
> On 16/11/2025 12:44, Jernej Škrabec wrote:
> > Hi!
> > 
> > Dne nedelja, 16. november 2025 ob 12:29:27 Srednjeevropski standardni čas je Krzysztof Kozlowski napisal(a):
> >> On Sat, Nov 15, 2025 at 03:13:44PM +0100, Jernej Skrabec wrote:
> >>> Allwinner Display Engine 3.3 contains planes, which are shared resources
> >>> between all mixers present in SoC. They can be assigned to specific
> >>> mixer by using registers which reside in display clocks MMIO.
> >>>
> >>> Add a binding for them.
> >>>
> >>> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> >>> ---
> >>>  .../allwinner,sun50i-h616-de33-planes.yaml    | 44 +++++++++++++++++++
> >>>  1 file changed, 44 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml b/Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml
> >>> new file mode 100644
> >>> index 000000000000..801e5068a6b5
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/display/allwinner,sun50i-h616-de33-planes.yaml
> >>> @@ -0,0 +1,44 @@
> >>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/display/allwinner,sun50i-h616-de33-planes.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Allwinner H616 Display Engine 3.3 planes
> >>> +
> >>> +maintainers:
> >>> +  - Jernej Skrabec <jernej.skrabec@gmail.com>
> >>> +
> >>> +description: |
> >>
> >> Do not need '|' unless you need to preserve formatting.
> >>
> >>> +  Display Engine 3.3 planes are independent of mixers, contrary to
> >>> +  previous generations of Display Engine. Planes can be assigned to
> >>> +  mixers independently and even dynamically during runtime.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - allwinner,sun50i-h616-de33-planes
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  allwinner,plane-mapping:
> >>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>> +    description: Phandle of Display Engine clock node
> >>
> >> You description is almost duplicating property name. You need to explain
> >> here how this device uses them.
> > 
> > So I guess I can copy commit description here? It is needed to
> > access registers from different core, so it can assign (map)
> > planes between mixers at runtime.
> 
> 
> "to assign (map) planes between mixers." is enough.
> 
> But it looks unfortunately like a spaghetti.
> 
> Your mixer binding references via phandle this planes. These planes
> reference via phandle some other region to configure planes between mixers.
> 
> Isn't this the job of this device?

It is a bit confusing, yes. There is no clean split in register space
for some functionality. Register space for this node on H616 SoC
represents 6 planes (each plane consist of framebuffer management, CSC
unit, scaler, etc.) but not actual registers which tell to which mixer
they are currently assigned.

Best regards,
Jernej

[PATCH 5/7] drm/sun4i: Add planes driver

[Jernej Skrabec]

This driver serves just as planes sharing manager, needed for Display
Engine 3.3 and newer.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/gpu/drm/sun4i/Kconfig         |   8 +
 drivers/gpu/drm/sun4i/Makefile        |   1 +
 drivers/gpu/drm/sun4i/sun50i_planes.c | 205 ++++++++++++++++++++++++++
 drivers/gpu/drm/sun4i/sun50i_planes.h |  43 ++++++
 4 files changed, 257 insertions(+)
 create mode 100644 drivers/gpu/drm/sun4i/sun50i_planes.c
 create mode 100644 drivers/gpu/drm/sun4i/sun50i_planes.h

diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
index b56ba00aabca..946dd7606094 100644
--- a/drivers/gpu/drm/sun4i/Kconfig
+++ b/drivers/gpu/drm/sun4i/Kconfig
@@ -85,4 +85,12 @@ config DRM_SUN8I_TCON_TOP
 	  TCON TOP is responsible for configuring display pipeline for
 	  HDMI, TVE and LCD.
 
+config DRM_SUN50I_PLANES
+	tristate
+	default DRM_SUN4I if DRM_SUN8I_MIXER!=n
+	help
+	  Chose this option if you have an Allwinner Soc with the
+	  Display Engine 3.3 or newer. Planes are shared resource
+	  between multiple mixers.
+
 endif
diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index bad7497a0d11..03f002abef15 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -38,3 +38,4 @@ obj-$(CONFIG_DRM_SUN6I_DSI)	+= sun6i_mipi_dsi.o
 obj-$(CONFIG_DRM_SUN8I_DW_HDMI)	+= sun8i-drm-hdmi.o
 obj-$(CONFIG_DRM_SUN8I_MIXER)	+= sun8i-mixer.o
 obj-$(CONFIG_DRM_SUN8I_TCON_TOP) += sun8i_tcon_top.o
+obj-$(CONFIG_DRM_SUN50I_PLANES)	+= sun50i_planes.o
diff --git a/drivers/gpu/drm/sun4i/sun50i_planes.c b/drivers/gpu/drm/sun4i/sun50i_planes.c
new file mode 100644
index 000000000000..a99c01122990
--- /dev/null
+++ b/drivers/gpu/drm/sun4i/sun50i_planes.c
@@ -0,0 +1,205 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* Copyright (c) 2025 Jernej Skrabec <jernej.skrabec@gmail.com> */
+
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/platform_device.h>
+
+#include "sun50i_planes.h"
+#include "sun8i_ui_layer.h"
+#include "sun8i_vi_layer.h"
+
+static bool sun50i_planes_node_is_planes(struct device_node *node)
+{
+	return !!of_match_node(sun50i_planes_of_table, node);
+}
+
+struct drm_plane **
+sun50i_planes_setup(struct device *dev, struct drm_device *drm,
+		    unsigned int mixer)
+{
+	struct sun50i_planes *planes = dev_get_drvdata(dev);
+	const struct sun50i_planes_quirks *quirks;
+	struct drm_plane **drm_planes;
+	const struct default_map *map;
+	unsigned int i;
+
+	if (!sun50i_planes_node_is_planes(dev->of_node)) {
+		dev_err(dev, "Device is not planes driver!\n");
+		return NULL;
+	}
+
+	if (!planes) {
+		dev_err(dev, "Planes driver is not loaded yet!\n");
+		return NULL;
+	}
+
+	if (mixer > 1) {
+		dev_err(dev, "Mixer index is too high!\n");
+		return NULL;
+	}
+
+	quirks = planes->quirks;
+	map = &quirks->def_map[mixer];
+
+	drm_planes = devm_kcalloc(drm->dev, map->num_ch + 1,
+				  sizeof(*drm_planes), GFP_KERNEL);
+	if (!drm_planes)
+		return ERR_PTR(-ENOMEM);
+
+	for (i = 0; i < map->num_ch; i++) {
+		unsigned int phy_ch = map->map[i];
+		struct sun8i_layer *layer;
+		enum drm_plane_type type;
+
+		if ((i == 0 && map->num_ch == 1) || i == 1)
+			type = DRM_PLANE_TYPE_PRIMARY;
+		else
+			type = DRM_PLANE_TYPE_OVERLAY;
+
+		if (phy_ch < UI_PLANE_OFFSET)
+			layer = sun8i_vi_layer_init_one(drm, type, planes->regs,
+							i, phy_ch, map->num_ch,
+							&quirks->cfg);
+		else
+			layer = sun8i_ui_layer_init_one(drm, type, planes->regs,
+							i, phy_ch, map->num_ch,
+							&quirks->cfg);
+
+		if (IS_ERR(layer)) {
+			dev_err(drm->dev,
+				"Couldn't initialize DRM plane\n");
+			return ERR_CAST(layer);
+		}
+
+		drm_planes[i] = &layer->plane;
+	}
+
+	return drm_planes;
+}
+EXPORT_SYMBOL(sun50i_planes_setup);
+
+static void sun50i_planes_init_mapping(struct sun50i_planes *planes)
+{
+	const struct sun50i_planes_quirks *quirks = planes->quirks;
+	unsigned int i, j;
+	u32 mapping;
+
+	mapping = 0;
+	for (j = 0; j < MAX_DISP; j++)
+		for (i = 0; i < quirks->def_map[j].num_ch; i++) {
+			unsigned int ch = quirks->def_map[j].map[i];
+
+			if (ch < UI_PLANE_OFFSET)
+				mapping |= j << (ch * 2);
+			else
+				mapping |= j << ((ch - UI_PLANE_OFFSET) * 2 + 16);
+		}
+	regmap_write(planes->mapping, SUNXI_DE33_DE_CHN2CORE_MUX_REG, mapping);
+
+	for (j = 0; j < MAX_DISP; j++) {
+		mapping = 0;
+		for (i = 0; i < quirks->def_map[j].num_ch; i++) {
+			unsigned int ch = quirks->def_map[j].map[i];
+
+			if (ch >= UI_PLANE_OFFSET)
+				ch += 2;
+
+			mapping |= ch << (i * 4);
+		}
+		regmap_write(planes->mapping, SUNXI_DE33_DE_PORT02CHN_MUX_REG + j * 4, mapping);
+	}
+}
+
+static const struct regmap_config sun50i_planes_regmap_config = {
+	.name		= "planes",
+	.reg_bits	= 32,
+	.val_bits	= 32,
+	.reg_stride	= 4,
+	.max_register	= 0x17fffc,
+};
+
+static int sun50i_planes_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sun50i_planes *planes;
+	void __iomem *regs;
+
+	planes = devm_kzalloc(dev, sizeof(*planes), GFP_KERNEL);
+	if (!planes)
+		return -ENOMEM;
+
+	planes->quirks = of_device_get_match_data(&pdev->dev);
+	if (!planes->quirks)
+		return dev_err_probe(dev, -EINVAL, "Unable to get quirks\n");
+
+	planes->mapping = syscon_regmap_lookup_by_phandle(dev->of_node,
+							  "allwinner,plane-mapping");
+	if (IS_ERR(planes->mapping))
+		return dev_err_probe(dev, PTR_ERR(planes->mapping),
+				     "Unable to get mapping\n");
+
+	regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
+
+	planes->regs = devm_regmap_init_mmio(dev, regs, &sun50i_planes_regmap_config);
+	if (IS_ERR(planes->regs))
+		return PTR_ERR(planes->regs);
+
+	dev_set_drvdata(dev, planes);
+
+	sun50i_planes_init_mapping(planes);
+
+	return 0;
+}
+
+static const struct sun50i_planes_quirks sun50i_h616_planes_quirks = {
+	.def_map = {
+		{
+			.map = {0, 6, 7},
+			.num_ch = 3,
+		},
+		{
+			.map = {1, 2, 8},
+			.num_ch = 3,
+		},
+	},
+	.cfg = {
+		.de_type	= SUN8I_MIXER_DE33,
+		/*
+		 * TODO: All planes support scaling, but driver needs
+		 * improvements to properly support it.
+		 */
+		.scaler_mask    = 0,
+		.scanline_yuv	= 4096,
+	},
+};
+
+/* sun4i_drv uses this list to check if a device node is a plane */
+const struct of_device_id sun50i_planes_of_table[] = {
+	{
+		.compatible = "allwinner,sun50i-h616-de33-planes",
+		.data = &sun50i_h616_planes_quirks
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sun50i_planes_of_table);
+EXPORT_SYMBOL(sun50i_planes_of_table);
+
+static struct platform_driver sun50i_planes_platform_driver = {
+	.probe		= sun50i_planes_probe,
+	.driver		= {
+		.name		= "sun50i-planes",
+		.of_match_table	= sun50i_planes_of_table,
+	},
+};
+module_platform_driver(sun50i_planes_platform_driver);
+
+MODULE_AUTHOR("Jernej Skrabec <jernej.skrabec@gmail.com>");
+MODULE_DESCRIPTION("Allwinner DE33 planes driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/sun4i/sun50i_planes.h b/drivers/gpu/drm/sun4i/sun50i_planes.h
new file mode 100644
index 000000000000..446feaeb03fc
--- /dev/null
+++ b/drivers/gpu/drm/sun4i/sun50i_planes.h
@@ -0,0 +1,43 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/* Copyright (c) 2025 Jernej Skrabec <jernej.skrabec@gmail.com> */
+
+#ifndef _SUN50I_PLANES_H_
+#define _SUN50I_PLANES_H_
+
+#include <drm/drm_device.h>
+#include <linux/regmap.h>
+
+#include "sun8i_mixer.h"
+
+/* mapping registers, located in clock register space */
+#define SUNXI_DE33_DE_CHN2CORE_MUX_REG	0x24
+#define SUNXI_DE33_DE_PORT02CHN_MUX_REG	0x28
+#define SUNXI_DE33_DE_PORT12CHN_MUX_REG	0x2c
+
+#define MAX_DISP	2
+#define MAX_CHANNELS	8
+#define UI_PLANE_OFFSET	6
+
+struct default_map {
+	unsigned int map[MAX_CHANNELS];
+	unsigned int num_ch;
+};
+
+struct sun50i_planes_quirks {
+	struct default_map	def_map[MAX_DISP];
+	struct sun8i_layer_cfg	cfg;
+};
+
+struct sun50i_planes {
+	struct regmap				*regs;
+	struct regmap				*mapping;
+	const struct sun50i_planes_quirks	*quirks;
+};
+
+extern const struct of_device_id sun50i_planes_of_table[];
+
+struct drm_plane **
+sun50i_planes_setup(struct device *dev, struct drm_device *drm,
+		    unsigned int mixer);
+
+#endif /* _SUN50I_PLANES_H_ */
-- 
2.51.2

Re: [PATCH 5/7] drm/sun4i: Add planes driver

[Chen-Yu Tsai]

On Sat, Nov 15, 2025 at 10:14 PM Jernej Skrabec
<jernej.skrabec@gmail.com> wrote:
>
> This driver serves just as planes sharing manager, needed for Display
> Engine 3.3 and newer.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/gpu/drm/sun4i/Kconfig         |   8 +
>  drivers/gpu/drm/sun4i/Makefile        |   1 +
>  drivers/gpu/drm/sun4i/sun50i_planes.c | 205 ++++++++++++++++++++++++++
>  drivers/gpu/drm/sun4i/sun50i_planes.h |  43 ++++++
>  4 files changed, 257 insertions(+)
>  create mode 100644 drivers/gpu/drm/sun4i/sun50i_planes.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun50i_planes.h
>
> diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
> index b56ba00aabca..946dd7606094 100644
> --- a/drivers/gpu/drm/sun4i/Kconfig
> +++ b/drivers/gpu/drm/sun4i/Kconfig
> @@ -85,4 +85,12 @@ config DRM_SUN8I_TCON_TOP
>           TCON TOP is responsible for configuring display pipeline for
>           HDMI, TVE and LCD.
>
> +config DRM_SUN50I_PLANES
> +       tristate
> +       default DRM_SUN4I if DRM_SUN8I_MIXER!=n
> +       help
> +         Chose this option if you have an Allwinner Soc with the
> +         Display Engine 3.3 or newer. Planes are shared resource
> +         between multiple mixers.
> +
>  endif
> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> index bad7497a0d11..03f002abef15 100644
> --- a/drivers/gpu/drm/sun4i/Makefile
> +++ b/drivers/gpu/drm/sun4i/Makefile
> @@ -38,3 +38,4 @@ obj-$(CONFIG_DRM_SUN6I_DSI)   += sun6i_mipi_dsi.o
>  obj-$(CONFIG_DRM_SUN8I_DW_HDMI)        += sun8i-drm-hdmi.o
>  obj-$(CONFIG_DRM_SUN8I_MIXER)  += sun8i-mixer.o
>  obj-$(CONFIG_DRM_SUN8I_TCON_TOP) += sun8i_tcon_top.o
> +obj-$(CONFIG_DRM_SUN50I_PLANES)        += sun50i_planes.o

I don't think you can have this as a separate module:

a. You are using sun8i_vi_layer_init_one() and sun8i_ui_layer_init_one()
   from the sun8i-mixer module, and neither of them are exported symbols.

b. You export sun50i_planes_setup() for sun8i-mixer to call, which ends
   up becoming a circular dependency.

The easiest solution would be to just fold this into the sun8i-mixer module.


> diff --git a/drivers/gpu/drm/sun4i/sun50i_planes.c b/drivers/gpu/drm/sun4i/sun50i_planes.c
> new file mode 100644
> index 000000000000..a99c01122990
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun50i_planes.c
> @@ -0,0 +1,205 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/* Copyright (c) 2025 Jernej Skrabec <jernej.skrabec@gmail.com> */
> +
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +
> +#include "sun50i_planes.h"
> +#include "sun8i_ui_layer.h"
> +#include "sun8i_vi_layer.h"
> +
> +static bool sun50i_planes_node_is_planes(struct device_node *node)
> +{
> +       return !!of_match_node(sun50i_planes_of_table, node);
> +}
> +
> +struct drm_plane **
> +sun50i_planes_setup(struct device *dev, struct drm_device *drm,
> +                   unsigned int mixer)
> +{
> +       struct sun50i_planes *planes = dev_get_drvdata(dev);
> +       const struct sun50i_planes_quirks *quirks;
> +       struct drm_plane **drm_planes;
> +       const struct default_map *map;
> +       unsigned int i;
> +
> +       if (!sun50i_planes_node_is_planes(dev->of_node)) {
> +               dev_err(dev, "Device is not planes driver!\n");
> +               return NULL;
> +       }
> +
> +       if (!planes) {
> +               dev_err(dev, "Planes driver is not loaded yet!\n");
> +               return NULL;
> +       }
> +
> +       if (mixer > 1) {
> +               dev_err(dev, "Mixer index is too high!\n");
> +               return NULL;
> +       }
> +
> +       quirks = planes->quirks;
> +       map = &quirks->def_map[mixer];
> +
> +       drm_planes = devm_kcalloc(drm->dev, map->num_ch + 1,

Just a note: it seems we are missing the sentinel in sun8i_layers_init().

> +                                 sizeof(*drm_planes), GFP_KERNEL);
> +       if (!drm_planes)
> +               return ERR_PTR(-ENOMEM);
> +
> +       for (i = 0; i < map->num_ch; i++) {
> +               unsigned int phy_ch = map->map[i];
> +               struct sun8i_layer *layer;
> +               enum drm_plane_type type;
> +
> +               if ((i == 0 && map->num_ch == 1) || i == 1)
> +                       type = DRM_PLANE_TYPE_PRIMARY;
> +               else
> +                       type = DRM_PLANE_TYPE_OVERLAY;
> +
> +               if (phy_ch < UI_PLANE_OFFSET)
> +                       layer = sun8i_vi_layer_init_one(drm, type, planes->regs,
> +                                                       i, phy_ch, map->num_ch,
> +                                                       &quirks->cfg);
> +               else
> +                       layer = sun8i_ui_layer_init_one(drm, type, planes->regs,
> +                                                       i, phy_ch, map->num_ch,
> +                                                       &quirks->cfg);
> +
> +               if (IS_ERR(layer)) {
> +                       dev_err(drm->dev,
> +                               "Couldn't initialize DRM plane\n");
> +                       return ERR_CAST(layer);
> +               }
> +
> +               drm_planes[i] = &layer->plane;
> +       }
> +
> +       return drm_planes;
> +}
> +EXPORT_SYMBOL(sun50i_planes_setup);
> +
> +static void sun50i_planes_init_mapping(struct sun50i_planes *planes)
> +{
> +       const struct sun50i_planes_quirks *quirks = planes->quirks;
> +       unsigned int i, j;
> +       u32 mapping;
> +
> +       mapping = 0;
> +       for (j = 0; j < MAX_DISP; j++)
> +               for (i = 0; i < quirks->def_map[j].num_ch; i++) {
> +                       unsigned int ch = quirks->def_map[j].map[i];
> +
> +                       if (ch < UI_PLANE_OFFSET)
> +                               mapping |= j << (ch * 2);
> +                       else
> +                               mapping |= j << ((ch - UI_PLANE_OFFSET) * 2 + 16);
> +               }
> +       regmap_write(planes->mapping, SUNXI_DE33_DE_CHN2CORE_MUX_REG, mapping);
> +
> +       for (j = 0; j < MAX_DISP; j++) {
> +               mapping = 0;
> +               for (i = 0; i < quirks->def_map[j].num_ch; i++) {
> +                       unsigned int ch = quirks->def_map[j].map[i];
> +
> +                       if (ch >= UI_PLANE_OFFSET)
> +                               ch += 2;
> +
> +                       mapping |= ch << (i * 4);
> +               }
> +               regmap_write(planes->mapping, SUNXI_DE33_DE_PORT02CHN_MUX_REG + j * 4, mapping);
> +       }
> +}
> +
> +static const struct regmap_config sun50i_planes_regmap_config = {
> +       .name           = "planes",
> +       .reg_bits       = 32,
> +       .val_bits       = 32,
> +       .reg_stride     = 4,
> +       .max_register   = 0x17fffc,
> +};
> +
> +static int sun50i_planes_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct sun50i_planes *planes;
> +       void __iomem *regs;
> +
> +       planes = devm_kzalloc(dev, sizeof(*planes), GFP_KERNEL);
> +       if (!planes)
> +               return -ENOMEM;
> +
> +       planes->quirks = of_device_get_match_data(&pdev->dev);
> +       if (!planes->quirks)
> +               return dev_err_probe(dev, -EINVAL, "Unable to get quirks\n");
> +
> +       planes->mapping = syscon_regmap_lookup_by_phandle(dev->of_node,
> +                                                         "allwinner,plane-mapping");
> +       if (IS_ERR(planes->mapping))
> +               return dev_err_probe(dev, PTR_ERR(planes->mapping),
> +                                    "Unable to get mapping\n");
> +
> +       regs = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(regs))
> +               return PTR_ERR(regs);
> +
> +       planes->regs = devm_regmap_init_mmio(dev, regs, &sun50i_planes_regmap_config);
> +       if (IS_ERR(planes->regs))
> +               return PTR_ERR(planes->regs);
> +
> +       dev_set_drvdata(dev, planes);
> +
> +       sun50i_planes_init_mapping(planes);
> +
> +       return 0;
> +}
> +
> +static const struct sun50i_planes_quirks sun50i_h616_planes_quirks = {
> +       .def_map = {
> +               {
> +                       .map = {0, 6, 7},
> +                       .num_ch = 3,
> +               },
> +               {
> +                       .map = {1, 2, 8},
> +                       .num_ch = 3,
> +               },
> +       },
> +       .cfg = {
> +               .de_type        = SUN8I_MIXER_DE33,
> +               /*
> +                * TODO: All planes support scaling, but driver needs
> +                * improvements to properly support it.
> +                */
> +               .scaler_mask    = 0,
> +               .scanline_yuv   = 4096,
> +       },
> +};
> +
> +/* sun4i_drv uses this list to check if a device node is a plane */
> +const struct of_device_id sun50i_planes_of_table[] = {
> +       {
> +               .compatible = "allwinner,sun50i-h616-de33-planes",
> +               .data = &sun50i_h616_planes_quirks
> +       },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, sun50i_planes_of_table);
> +EXPORT_SYMBOL(sun50i_planes_of_table);
> +
> +static struct platform_driver sun50i_planes_platform_driver = {
> +       .probe          = sun50i_planes_probe,
> +       .driver         = {
> +               .name           = "sun50i-planes",
> +               .of_match_table = sun50i_planes_of_table,
> +       },
> +};
> +module_platform_driver(sun50i_planes_platform_driver);
> +
> +MODULE_AUTHOR("Jernej Skrabec <jernej.skrabec@gmail.com>");
> +MODULE_DESCRIPTION("Allwinner DE33 planes driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/gpu/drm/sun4i/sun50i_planes.h b/drivers/gpu/drm/sun4i/sun50i_planes.h
> new file mode 100644
> index 000000000000..446feaeb03fc
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun50i_planes.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/* Copyright (c) 2025 Jernej Skrabec <jernej.skrabec@gmail.com> */
> +
> +#ifndef _SUN50I_PLANES_H_
> +#define _SUN50I_PLANES_H_
> +
> +#include <drm/drm_device.h>
> +#include <linux/regmap.h>

I think you could move these two to the .c file, and just use forward
declarations here.

The rest looks OK.


> +
> +#include "sun8i_mixer.h"
> +
> +/* mapping registers, located in clock register space */
> +#define SUNXI_DE33_DE_CHN2CORE_MUX_REG 0x24
> +#define SUNXI_DE33_DE_PORT02CHN_MUX_REG        0x28
> +#define SUNXI_DE33_DE_PORT12CHN_MUX_REG        0x2c
> +
> +#define MAX_DISP       2
> +#define MAX_CHANNELS   8
> +#define UI_PLANE_OFFSET        6
> +
> +struct default_map {
> +       unsigned int map[MAX_CHANNELS];
> +       unsigned int num_ch;
> +};
> +
> +struct sun50i_planes_quirks {
> +       struct default_map      def_map[MAX_DISP];
> +       struct sun8i_layer_cfg  cfg;
> +};
> +
> +struct sun50i_planes {
> +       struct regmap                           *regs;
> +       struct regmap                           *mapping;
> +       const struct sun50i_planes_quirks       *quirks;
> +};
> +
> +extern const struct of_device_id sun50i_planes_of_table[];
> +
> +struct drm_plane **
> +sun50i_planes_setup(struct device *dev, struct drm_device *drm,
> +                   unsigned int mixer);
> +
> +#endif /* _SUN50I_PLANES_H_ */
> --
> 2.51.2
>
>

Re: [PATCH 5/7] drm/sun4i: Add planes driver

[Chen-Yu Tsai]

On Thu, Dec 25, 2025 at 5:29 PM Chen-Yu Tsai <wens@kernel.org> wrote:
>
> On Sat, Nov 15, 2025 at 10:14 PM Jernej Skrabec
> <jernej.skrabec@gmail.com> wrote:
> >
> > This driver serves just as planes sharing manager, needed for Display
> > Engine 3.3 and newer.
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > ---
> >  drivers/gpu/drm/sun4i/Kconfig         |   8 +
> >  drivers/gpu/drm/sun4i/Makefile        |   1 +
> >  drivers/gpu/drm/sun4i/sun50i_planes.c | 205 ++++++++++++++++++++++++++
> >  drivers/gpu/drm/sun4i/sun50i_planes.h |  43 ++++++
> >  4 files changed, 257 insertions(+)
> >  create mode 100644 drivers/gpu/drm/sun4i/sun50i_planes.c
> >  create mode 100644 drivers/gpu/drm/sun4i/sun50i_planes.h
> >
> > diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
> > index b56ba00aabca..946dd7606094 100644
> > --- a/drivers/gpu/drm/sun4i/Kconfig
> > +++ b/drivers/gpu/drm/sun4i/Kconfig
> > @@ -85,4 +85,12 @@ config DRM_SUN8I_TCON_TOP
> >           TCON TOP is responsible for configuring display pipeline for
> >           HDMI, TVE and LCD.
> >
> > +config DRM_SUN50I_PLANES
> > +       tristate
> > +       default DRM_SUN4I if DRM_SUN8I_MIXER!=n
> > +       help
> > +         Chose this option if you have an Allwinner Soc with the
> > +         Display Engine 3.3 or newer. Planes are shared resource
> > +         between multiple mixers.
> > +
> >  endif
> > diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> > index bad7497a0d11..03f002abef15 100644
> > --- a/drivers/gpu/drm/sun4i/Makefile
> > +++ b/drivers/gpu/drm/sun4i/Makefile
> > @@ -38,3 +38,4 @@ obj-$(CONFIG_DRM_SUN6I_DSI)   += sun6i_mipi_dsi.o
> >  obj-$(CONFIG_DRM_SUN8I_DW_HDMI)        += sun8i-drm-hdmi.o
> >  obj-$(CONFIG_DRM_SUN8I_MIXER)  += sun8i-mixer.o
> >  obj-$(CONFIG_DRM_SUN8I_TCON_TOP) += sun8i_tcon_top.o
> > +obj-$(CONFIG_DRM_SUN50I_PLANES)        += sun50i_planes.o
>
> I don't think you can have this as a separate module:
>
> a. You are using sun8i_vi_layer_init_one() and sun8i_ui_layer_init_one()
>    from the sun8i-mixer module, and neither of them are exported symbols.
>
> b. You export sun50i_planes_setup() for sun8i-mixer to call, which ends
>    up becoming a circular dependency.
>
> The easiest solution would be to just fold this into the sun8i-mixer module.
>
>
> > diff --git a/drivers/gpu/drm/sun4i/sun50i_planes.c b/drivers/gpu/drm/sun4i/sun50i_planes.c
> > new file mode 100644
> > index 000000000000..a99c01122990
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sun4i/sun50i_planes.c
> > @@ -0,0 +1,205 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/* Copyright (c) 2025 Jernej Skrabec <jernej.skrabec@gmail.com> */
> > +
> > +#include <linux/device.h>
> > +#include <linux/io.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_graph.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include "sun50i_planes.h"
> > +#include "sun8i_ui_layer.h"
> > +#include "sun8i_vi_layer.h"
> > +
> > +static bool sun50i_planes_node_is_planes(struct device_node *node)
> > +{
> > +       return !!of_match_node(sun50i_planes_of_table, node);
> > +}
> > +
> > +struct drm_plane **
> > +sun50i_planes_setup(struct device *dev, struct drm_device *drm,
> > +                   unsigned int mixer)
> > +{
> > +       struct sun50i_planes *planes = dev_get_drvdata(dev);
> > +       const struct sun50i_planes_quirks *quirks;
> > +       struct drm_plane **drm_planes;
> > +       const struct default_map *map;
> > +       unsigned int i;
> > +
> > +       if (!sun50i_planes_node_is_planes(dev->of_node)) {
> > +               dev_err(dev, "Device is not planes driver!\n");
> > +               return NULL;
> > +       }
> > +
> > +       if (!planes) {
> > +               dev_err(dev, "Planes driver is not loaded yet!\n");
> > +               return NULL;
> > +       }
> > +
> > +       if (mixer > 1) {
> > +               dev_err(dev, "Mixer index is too high!\n");
> > +               return NULL;
> > +       }
> > +
> > +       quirks = planes->quirks;
> > +       map = &quirks->def_map[mixer];
> > +
> > +       drm_planes = devm_kcalloc(drm->dev, map->num_ch + 1,
>
> Just a note: it seems we are missing the sentinel in sun8i_layers_init().
>
> > +                                 sizeof(*drm_planes), GFP_KERNEL);
> > +       if (!drm_planes)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       for (i = 0; i < map->num_ch; i++) {
> > +               unsigned int phy_ch = map->map[i];
> > +               struct sun8i_layer *layer;
> > +               enum drm_plane_type type;
> > +
> > +               if ((i == 0 && map->num_ch == 1) || i == 1)
> > +                       type = DRM_PLANE_TYPE_PRIMARY;
> > +               else
> > +                       type = DRM_PLANE_TYPE_OVERLAY;
> > +
> > +               if (phy_ch < UI_PLANE_OFFSET)
> > +                       layer = sun8i_vi_layer_init_one(drm, type, planes->regs,
> > +                                                       i, phy_ch, map->num_ch,
> > +                                                       &quirks->cfg);
> > +               else
> > +                       layer = sun8i_ui_layer_init_one(drm, type, planes->regs,
> > +                                                       i, phy_ch, map->num_ch,
> > +                                                       &quirks->cfg);
> > +
> > +               if (IS_ERR(layer)) {
> > +                       dev_err(drm->dev,
> > +                               "Couldn't initialize DRM plane\n");
> > +                       return ERR_CAST(layer);
> > +               }
> > +
> > +               drm_planes[i] = &layer->plane;
> > +       }
> > +
> > +       return drm_planes;
> > +}
> > +EXPORT_SYMBOL(sun50i_planes_setup);
> > +
> > +static void sun50i_planes_init_mapping(struct sun50i_planes *planes)
> > +{
> > +       const struct sun50i_planes_quirks *quirks = planes->quirks;
> > +       unsigned int i, j;
> > +       u32 mapping;
> > +
> > +       mapping = 0;
> > +       for (j = 0; j < MAX_DISP; j++)
> > +               for (i = 0; i < quirks->def_map[j].num_ch; i++) {
> > +                       unsigned int ch = quirks->def_map[j].map[i];
> > +
> > +                       if (ch < UI_PLANE_OFFSET)
> > +                               mapping |= j << (ch * 2);
> > +                       else
> > +                               mapping |= j << ((ch - UI_PLANE_OFFSET) * 2 + 16);
> > +               }
> > +       regmap_write(planes->mapping, SUNXI_DE33_DE_CHN2CORE_MUX_REG, mapping);
> > +
> > +       for (j = 0; j < MAX_DISP; j++) {
> > +               mapping = 0;
> > +               for (i = 0; i < quirks->def_map[j].num_ch; i++) {
> > +                       unsigned int ch = quirks->def_map[j].map[i];
> > +
> > +                       if (ch >= UI_PLANE_OFFSET)
> > +                               ch += 2;
> > +
> > +                       mapping |= ch << (i * 4);
> > +               }
> > +               regmap_write(planes->mapping, SUNXI_DE33_DE_PORT02CHN_MUX_REG + j * 4, mapping);
> > +       }
> > +}
> > +
> > +static const struct regmap_config sun50i_planes_regmap_config = {
> > +       .name           = "planes",
> > +       .reg_bits       = 32,
> > +       .val_bits       = 32,
> > +       .reg_stride     = 4,
> > +       .max_register   = 0x17fffc,
> > +};
> > +
> > +static int sun50i_planes_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct sun50i_planes *planes;
> > +       void __iomem *regs;
> > +
> > +       planes = devm_kzalloc(dev, sizeof(*planes), GFP_KERNEL);
> > +       if (!planes)
> > +               return -ENOMEM;
> > +
> > +       planes->quirks = of_device_get_match_data(&pdev->dev);
> > +       if (!planes->quirks)
> > +               return dev_err_probe(dev, -EINVAL, "Unable to get quirks\n");
> > +
> > +       planes->mapping = syscon_regmap_lookup_by_phandle(dev->of_node,
> > +                                                         "allwinner,plane-mapping");
> > +       if (IS_ERR(planes->mapping))
> > +               return dev_err_probe(dev, PTR_ERR(planes->mapping),
> > +                                    "Unable to get mapping\n");
> > +
> > +       regs = devm_platform_ioremap_resource(pdev, 0);
> > +       if (IS_ERR(regs))
> > +               return PTR_ERR(regs);
> > +
> > +       planes->regs = devm_regmap_init_mmio(dev, regs, &sun50i_planes_regmap_config);
> > +       if (IS_ERR(planes->regs))
> > +               return PTR_ERR(planes->regs);
> > +
> > +       dev_set_drvdata(dev, planes);
> > +
> > +       sun50i_planes_init_mapping(planes);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct sun50i_planes_quirks sun50i_h616_planes_quirks = {
> > +       .def_map = {
> > +               {
> > +                       .map = {0, 6, 7},
> > +                       .num_ch = 3,
> > +               },
> > +               {
> > +                       .map = {1, 2, 8},
> > +                       .num_ch = 3,
> > +               },
> > +       },
> > +       .cfg = {
> > +               .de_type        = SUN8I_MIXER_DE33,
> > +               /*
> > +                * TODO: All planes support scaling, but driver needs
> > +                * improvements to properly support it.
> > +                */
> > +               .scaler_mask    = 0,
> > +               .scanline_yuv   = 4096,
> > +       },
> > +};
> > +
> > +/* sun4i_drv uses this list to check if a device node is a plane */

I didn't see any usage of this in patch 7. Is this part of another
series?

Maybe just export sun50i_planes_node_is_planes() instead?

> > +const struct of_device_id sun50i_planes_of_table[] = {
> > +       {
> > +               .compatible = "allwinner,sun50i-h616-de33-planes",
> > +               .data = &sun50i_h616_planes_quirks
> > +       },
> > +       { }
> > +};
> > +MODULE_DEVICE_TABLE(of, sun50i_planes_of_table);
> > +EXPORT_SYMBOL(sun50i_planes_of_table);
> > +
> > +static struct platform_driver sun50i_planes_platform_driver = {
> > +       .probe          = sun50i_planes_probe,
> > +       .driver         = {
> > +               .name           = "sun50i-planes",
> > +               .of_match_table = sun50i_planes_of_table,
> > +       },
> > +};
> > +module_platform_driver(sun50i_planes_platform_driver);
> > +
> > +MODULE_AUTHOR("Jernej Skrabec <jernej.skrabec@gmail.com>");
> > +MODULE_DESCRIPTION("Allwinner DE33 planes driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/gpu/drm/sun4i/sun50i_planes.h b/drivers/gpu/drm/sun4i/sun50i_planes.h
> > new file mode 100644
> > index 000000000000..446feaeb03fc
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sun4i/sun50i_planes.h
> > @@ -0,0 +1,43 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/* Copyright (c) 2025 Jernej Skrabec <jernej.skrabec@gmail.com> */
> > +
> > +#ifndef _SUN50I_PLANES_H_
> > +#define _SUN50I_PLANES_H_
> > +
> > +#include <drm/drm_device.h>
> > +#include <linux/regmap.h>
>
> I think you could move these two to the .c file, and just use forward
> declarations here.
>
> The rest looks OK.
>
>
> > +
> > +#include "sun8i_mixer.h"
> > +
> > +/* mapping registers, located in clock register space */
> > +#define SUNXI_DE33_DE_CHN2CORE_MUX_REG 0x24
> > +#define SUNXI_DE33_DE_PORT02CHN_MUX_REG        0x28
> > +#define SUNXI_DE33_DE_PORT12CHN_MUX_REG        0x2c
> > +
> > +#define MAX_DISP       2
> > +#define MAX_CHANNELS   8
> > +#define UI_PLANE_OFFSET        6
> > +
> > +struct default_map {
> > +       unsigned int map[MAX_CHANNELS];
> > +       unsigned int num_ch;
> > +};
> > +
> > +struct sun50i_planes_quirks {
> > +       struct default_map      def_map[MAX_DISP];
> > +       struct sun8i_layer_cfg  cfg;
> > +};
> > +
> > +struct sun50i_planes {
> > +       struct regmap                           *regs;
> > +       struct regmap                           *mapping;
> > +       const struct sun50i_planes_quirks       *quirks;
> > +};
> > +
> > +extern const struct of_device_id sun50i_planes_of_table[];
> > +
> > +struct drm_plane **
> > +sun50i_planes_setup(struct device *dev, struct drm_device *drm,
> > +                   unsigned int mixer);
> > +
> > +#endif /* _SUN50I_PLANES_H_ */
> > --
> > 2.51.2
> >
> >

Re: [PATCH 5/7] drm/sun4i: Add planes driver

[Jernej Škrabec]

Dne četrtek, 25. december 2025 ob 10:37:06 Srednjeevropski standardni čas je Chen-Yu Tsai napisal(a):
> On Thu, Dec 25, 2025 at 5:29 PM Chen-Yu Tsai <wens@kernel.org> wrote:
> >
> > On Sat, Nov 15, 2025 at 10:14 PM Jernej Skrabec
> > <jernej.skrabec@gmail.com> wrote:
> > >
> > > This driver serves just as planes sharing manager, needed for Display
> > > Engine 3.3 and newer.
> > >
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > > ---
> > >  drivers/gpu/drm/sun4i/Kconfig         |   8 +
> > >  drivers/gpu/drm/sun4i/Makefile        |   1 +
> > >  drivers/gpu/drm/sun4i/sun50i_planes.c | 205 ++++++++++++++++++++++++++
> > >  drivers/gpu/drm/sun4i/sun50i_planes.h |  43 ++++++
> > >  4 files changed, 257 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/sun4i/sun50i_planes.c
> > >  create mode 100644 drivers/gpu/drm/sun4i/sun50i_planes.h
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
> > > index b56ba00aabca..946dd7606094 100644
> > > --- a/drivers/gpu/drm/sun4i/Kconfig
> > > +++ b/drivers/gpu/drm/sun4i/Kconfig
> > > @@ -85,4 +85,12 @@ config DRM_SUN8I_TCON_TOP
> > >           TCON TOP is responsible for configuring display pipeline for
> > >           HDMI, TVE and LCD.
> > >
> > > +config DRM_SUN50I_PLANES
> > > +       tristate
> > > +       default DRM_SUN4I if DRM_SUN8I_MIXER!=n
> > > +       help
> > > +         Chose this option if you have an Allwinner Soc with the
> > > +         Display Engine 3.3 or newer. Planes are shared resource
> > > +         between multiple mixers.
> > > +
> > >  endif
> > > diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> > > index bad7497a0d11..03f002abef15 100644
> > > --- a/drivers/gpu/drm/sun4i/Makefile
> > > +++ b/drivers/gpu/drm/sun4i/Makefile
> > > @@ -38,3 +38,4 @@ obj-$(CONFIG_DRM_SUN6I_DSI)   += sun6i_mipi_dsi.o
> > >  obj-$(CONFIG_DRM_SUN8I_DW_HDMI)        += sun8i-drm-hdmi.o
> > >  obj-$(CONFIG_DRM_SUN8I_MIXER)  += sun8i-mixer.o
> > >  obj-$(CONFIG_DRM_SUN8I_TCON_TOP) += sun8i_tcon_top.o
> > > +obj-$(CONFIG_DRM_SUN50I_PLANES)        += sun50i_planes.o
> >
> > I don't think you can have this as a separate module:
> >
> > a. You are using sun8i_vi_layer_init_one() and sun8i_ui_layer_init_one()
> >    from the sun8i-mixer module, and neither of them are exported symbols.
> >
> > b. You export sun50i_planes_setup() for sun8i-mixer to call, which ends
> >    up becoming a circular dependency.
> >
> > The easiest solution would be to just fold this into the sun8i-mixer module.

I mimicked tcon-top module, but yeah, it's much less of a hassle to fold it
into sun8i-mixer.

> >
> >
> > > diff --git a/drivers/gpu/drm/sun4i/sun50i_planes.c b/drivers/gpu/drm/sun4i/sun50i_planes.c
> > > new file mode 100644
> > > index 000000000000..a99c01122990
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/sun4i/sun50i_planes.c
> > > @@ -0,0 +1,205 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/* Copyright (c) 2025 Jernej Skrabec <jernej.skrabec@gmail.com> */
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/io.h>
> > > +#include <linux/mfd/syscon.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_graph.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +#include "sun50i_planes.h"
> > > +#include "sun8i_ui_layer.h"
> > > +#include "sun8i_vi_layer.h"
> > > +
> > > +static bool sun50i_planes_node_is_planes(struct device_node *node)
> > > +{
> > > +       return !!of_match_node(sun50i_planes_of_table, node);
> > > +}
> > > +
> > > +struct drm_plane **
> > > +sun50i_planes_setup(struct device *dev, struct drm_device *drm,
> > > +                   unsigned int mixer)
> > > +{
> > > +       struct sun50i_planes *planes = dev_get_drvdata(dev);
> > > +       const struct sun50i_planes_quirks *quirks;
> > > +       struct drm_plane **drm_planes;
> > > +       const struct default_map *map;
> > > +       unsigned int i;
> > > +
> > > +       if (!sun50i_planes_node_is_planes(dev->of_node)) {
> > > +               dev_err(dev, "Device is not planes driver!\n");
> > > +               return NULL;
> > > +       }
> > > +
> > > +       if (!planes) {
> > > +               dev_err(dev, "Planes driver is not loaded yet!\n");
> > > +               return NULL;
> > > +       }
> > > +
> > > +       if (mixer > 1) {
> > > +               dev_err(dev, "Mixer index is too high!\n");
> > > +               return NULL;
> > > +       }
> > > +
> > > +       quirks = planes->quirks;
> > > +       map = &quirks->def_map[mixer];
> > > +
> > > +       drm_planes = devm_kcalloc(drm->dev, map->num_ch + 1,
> >
> > Just a note: it seems we are missing the sentinel in sun8i_layers_init().

Why do you think so? Current mainline code has mixer->cfg->vi_num +
mixer->cfg->ui_num + 1.

> >
> > > +                                 sizeof(*drm_planes), GFP_KERNEL);
> > > +       if (!drm_planes)
> > > +               return ERR_PTR(-ENOMEM);
> > > +
> > > +       for (i = 0; i < map->num_ch; i++) {
> > > +               unsigned int phy_ch = map->map[i];
> > > +               struct sun8i_layer *layer;
> > > +               enum drm_plane_type type;
> > > +
> > > +               if ((i == 0 && map->num_ch == 1) || i == 1)
> > > +                       type = DRM_PLANE_TYPE_PRIMARY;
> > > +               else
> > > +                       type = DRM_PLANE_TYPE_OVERLAY;
> > > +
> > > +               if (phy_ch < UI_PLANE_OFFSET)
> > > +                       layer = sun8i_vi_layer_init_one(drm, type, planes->regs,
> > > +                                                       i, phy_ch, map->num_ch,
> > > +                                                       &quirks->cfg);
> > > +               else
> > > +                       layer = sun8i_ui_layer_init_one(drm, type, planes->regs,
> > > +                                                       i, phy_ch, map->num_ch,
> > > +                                                       &quirks->cfg);
> > > +
> > > +               if (IS_ERR(layer)) {
> > > +                       dev_err(drm->dev,
> > > +                               "Couldn't initialize DRM plane\n");
> > > +                       return ERR_CAST(layer);
> > > +               }
> > > +
> > > +               drm_planes[i] = &layer->plane;
> > > +       }
> > > +
> > > +       return drm_planes;
> > > +}
> > > +EXPORT_SYMBOL(sun50i_planes_setup);
> > > +
> > > +static void sun50i_planes_init_mapping(struct sun50i_planes *planes)
> > > +{
> > > +       const struct sun50i_planes_quirks *quirks = planes->quirks;
> > > +       unsigned int i, j;
> > > +       u32 mapping;
> > > +
> > > +       mapping = 0;
> > > +       for (j = 0; j < MAX_DISP; j++)
> > > +               for (i = 0; i < quirks->def_map[j].num_ch; i++) {
> > > +                       unsigned int ch = quirks->def_map[j].map[i];
> > > +
> > > +                       if (ch < UI_PLANE_OFFSET)
> > > +                               mapping |= j << (ch * 2);
> > > +                       else
> > > +                               mapping |= j << ((ch - UI_PLANE_OFFSET) * 2 + 16);
> > > +               }
> > > +       regmap_write(planes->mapping, SUNXI_DE33_DE_CHN2CORE_MUX_REG, mapping);
> > > +
> > > +       for (j = 0; j < MAX_DISP; j++) {
> > > +               mapping = 0;
> > > +               for (i = 0; i < quirks->def_map[j].num_ch; i++) {
> > > +                       unsigned int ch = quirks->def_map[j].map[i];
> > > +
> > > +                       if (ch >= UI_PLANE_OFFSET)
> > > +                               ch += 2;
> > > +
> > > +                       mapping |= ch << (i * 4);
> > > +               }
> > > +               regmap_write(planes->mapping, SUNXI_DE33_DE_PORT02CHN_MUX_REG + j * 4, mapping);
> > > +       }
> > > +}
> > > +
> > > +static const struct regmap_config sun50i_planes_regmap_config = {
> > > +       .name           = "planes",
> > > +       .reg_bits       = 32,
> > > +       .val_bits       = 32,
> > > +       .reg_stride     = 4,
> > > +       .max_register   = 0x17fffc,
> > > +};
> > > +
> > > +static int sun50i_planes_probe(struct platform_device *pdev)
> > > +{
> > > +       struct device *dev = &pdev->dev;
> > > +       struct sun50i_planes *planes;
> > > +       void __iomem *regs;
> > > +
> > > +       planes = devm_kzalloc(dev, sizeof(*planes), GFP_KERNEL);
> > > +       if (!planes)
> > > +               return -ENOMEM;
> > > +
> > > +       planes->quirks = of_device_get_match_data(&pdev->dev);
> > > +       if (!planes->quirks)
> > > +               return dev_err_probe(dev, -EINVAL, "Unable to get quirks\n");
> > > +
> > > +       planes->mapping = syscon_regmap_lookup_by_phandle(dev->of_node,
> > > +                                                         "allwinner,plane-mapping");
> > > +       if (IS_ERR(planes->mapping))
> > > +               return dev_err_probe(dev, PTR_ERR(planes->mapping),
> > > +                                    "Unable to get mapping\n");
> > > +
> > > +       regs = devm_platform_ioremap_resource(pdev, 0);
> > > +       if (IS_ERR(regs))
> > > +               return PTR_ERR(regs);
> > > +
> > > +       planes->regs = devm_regmap_init_mmio(dev, regs, &sun50i_planes_regmap_config);
> > > +       if (IS_ERR(planes->regs))
> > > +               return PTR_ERR(planes->regs);
> > > +
> > > +       dev_set_drvdata(dev, planes);
> > > +
> > > +       sun50i_planes_init_mapping(planes);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static const struct sun50i_planes_quirks sun50i_h616_planes_quirks = {
> > > +       .def_map = {
> > > +               {
> > > +                       .map = {0, 6, 7},
> > > +                       .num_ch = 3,
> > > +               },
> > > +               {
> > > +                       .map = {1, 2, 8},
> > > +                       .num_ch = 3,
> > > +               },
> > > +       },
> > > +       .cfg = {
> > > +               .de_type        = SUN8I_MIXER_DE33,
> > > +               /*
> > > +                * TODO: All planes support scaling, but driver needs
> > > +                * improvements to properly support it.
> > > +                */
> > > +               .scaler_mask    = 0,
> > > +               .scanline_yuv   = 4096,
> > > +       },
> > > +};
> > > +
> > > +/* sun4i_drv uses this list to check if a device node is a plane */
> 
> I didn't see any usage of this in patch 7. Is this part of another
> series?
> 
> Maybe just export sun50i_planes_node_is_planes() instead?

It's not needed if driver is folder into sun8i-mixer module. Looks like this
is remnant of tcon-top concept...

> 
> > > +const struct of_device_id sun50i_planes_of_table[] = {
> > > +       {
> > > +               .compatible = "allwinner,sun50i-h616-de33-planes",
> > > +               .data = &sun50i_h616_planes_quirks
> > > +       },
> > > +       { }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, sun50i_planes_of_table);
> > > +EXPORT_SYMBOL(sun50i_planes_of_table);
> > > +
> > > +static struct platform_driver sun50i_planes_platform_driver = {
> > > +       .probe          = sun50i_planes_probe,
> > > +       .driver         = {
> > > +               .name           = "sun50i-planes",
> > > +               .of_match_table = sun50i_planes_of_table,
> > > +       },
> > > +};
> > > +module_platform_driver(sun50i_planes_platform_driver);
> > > +
> > > +MODULE_AUTHOR("Jernej Skrabec <jernej.skrabec@gmail.com>");
> > > +MODULE_DESCRIPTION("Allwinner DE33 planes driver");
> > > +MODULE_LICENSE("GPL");
> > > diff --git a/drivers/gpu/drm/sun4i/sun50i_planes.h b/drivers/gpu/drm/sun4i/sun50i_planes.h
> > > new file mode 100644
> > > index 000000000000..446feaeb03fc
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/sun4i/sun50i_planes.h
> > > @@ -0,0 +1,43 @@
> > > +/* SPDX-License-Identifier: GPL-2.0+ */
> > > +/* Copyright (c) 2025 Jernej Skrabec <jernej.skrabec@gmail.com> */
> > > +
> > > +#ifndef _SUN50I_PLANES_H_
> > > +#define _SUN50I_PLANES_H_
> > > +
> > > +#include <drm/drm_device.h>
> > > +#include <linux/regmap.h>
> >
> > I think you could move these two to the .c file, and just use forward
> > declarations here.

Ok.

Best regards,
Jernej

> >
> > The rest looks OK.
> >
> >
> > > +
> > > +#include "sun8i_mixer.h"
> > > +
> > > +/* mapping registers, located in clock register space */
> > > +#define SUNXI_DE33_DE_CHN2CORE_MUX_REG 0x24
> > > +#define SUNXI_DE33_DE_PORT02CHN_MUX_REG        0x28
> > > +#define SUNXI_DE33_DE_PORT12CHN_MUX_REG        0x2c
> > > +
> > > +#define MAX_DISP       2
> > > +#define MAX_CHANNELS   8
> > > +#define UI_PLANE_OFFSET        6
> > > +
> > > +struct default_map {
> > > +       unsigned int map[MAX_CHANNELS];
> > > +       unsigned int num_ch;
> > > +};
> > > +
> > > +struct sun50i_planes_quirks {
> > > +       struct default_map      def_map[MAX_DISP];
> > > +       struct sun8i_layer_cfg  cfg;
> > > +};
> > > +
> > > +struct sun50i_planes {
> > > +       struct regmap                           *regs;
> > > +       struct regmap                           *mapping;
> > > +       const struct sun50i_planes_quirks       *quirks;
> > > +};
> > > +
> > > +extern const struct of_device_id sun50i_planes_of_table[];
> > > +
> > > +struct drm_plane **
> > > +sun50i_planes_setup(struct device *dev, struct drm_device *drm,
> > > +                   unsigned int mixer);
> > > +
> > > +#endif /* _SUN50I_PLANES_H_ */
> > > --
> > > 2.51.2
> > >
> > >
> 

Re: [PATCH 5/7] drm/sun4i: Add planes driver

[Chen-Yu Tsai]

On Fri, Dec 26, 2025 at 3:17 AM Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
>
> Dne četrtek, 25. december 2025 ob 10:37:06 Srednjeevropski standardni čas je Chen-Yu Tsai napisal(a):
> > On Thu, Dec 25, 2025 at 5:29 PM Chen-Yu Tsai <wens@kernel.org> wrote:
> > >
> > > On Sat, Nov 15, 2025 at 10:14 PM Jernej Skrabec
> > > <jernej.skrabec@gmail.com> wrote:
> > > >
> > > > This driver serves just as planes sharing manager, needed for Display
> > > > Engine 3.3 and newer.
> > > >
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > > > ---
> > > >  drivers/gpu/drm/sun4i/Kconfig         |   8 +
> > > >  drivers/gpu/drm/sun4i/Makefile        |   1 +
> > > >  drivers/gpu/drm/sun4i/sun50i_planes.c | 205 ++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/sun4i/sun50i_planes.h |  43 ++++++
> > > >  4 files changed, 257 insertions(+)
> > > >  create mode 100644 drivers/gpu/drm/sun4i/sun50i_planes.c
> > > >  create mode 100644 drivers/gpu/drm/sun4i/sun50i_planes.h
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
> > > > index b56ba00aabca..946dd7606094 100644
> > > > --- a/drivers/gpu/drm/sun4i/Kconfig
> > > > +++ b/drivers/gpu/drm/sun4i/Kconfig
> > > > @@ -85,4 +85,12 @@ config DRM_SUN8I_TCON_TOP
> > > >           TCON TOP is responsible for configuring display pipeline for
> > > >           HDMI, TVE and LCD.
> > > >
> > > > +config DRM_SUN50I_PLANES
> > > > +       tristate
> > > > +       default DRM_SUN4I if DRM_SUN8I_MIXER!=n
> > > > +       help
> > > > +         Chose this option if you have an Allwinner Soc with the
> > > > +         Display Engine 3.3 or newer. Planes are shared resource
> > > > +         between multiple mixers.
> > > > +
> > > >  endif
> > > > diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> > > > index bad7497a0d11..03f002abef15 100644
> > > > --- a/drivers/gpu/drm/sun4i/Makefile
> > > > +++ b/drivers/gpu/drm/sun4i/Makefile
> > > > @@ -38,3 +38,4 @@ obj-$(CONFIG_DRM_SUN6I_DSI)   += sun6i_mipi_dsi.o
> > > >  obj-$(CONFIG_DRM_SUN8I_DW_HDMI)        += sun8i-drm-hdmi.o
> > > >  obj-$(CONFIG_DRM_SUN8I_MIXER)  += sun8i-mixer.o
> > > >  obj-$(CONFIG_DRM_SUN8I_TCON_TOP) += sun8i_tcon_top.o
> > > > +obj-$(CONFIG_DRM_SUN50I_PLANES)        += sun50i_planes.o
> > >
> > > I don't think you can have this as a separate module:
> > >
> > > a. You are using sun8i_vi_layer_init_one() and sun8i_ui_layer_init_one()
> > >    from the sun8i-mixer module, and neither of them are exported symbols.
> > >
> > > b. You export sun50i_planes_setup() for sun8i-mixer to call, which ends
> > >    up becoming a circular dependency.
> > >
> > > The easiest solution would be to just fold this into the sun8i-mixer module.
>
> I mimicked tcon-top module, but yeah, it's much less of a hassle to fold it
> into sun8i-mixer.
>
> > >
> > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun50i_planes.c b/drivers/gpu/drm/sun4i/sun50i_planes.c
> > > > new file mode 100644
> > > > index 000000000000..a99c01122990
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/sun4i/sun50i_planes.c
> > > > @@ -0,0 +1,205 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/* Copyright (c) 2025 Jernej Skrabec <jernej.skrabec@gmail.com> */
> > > > +
> > > > +#include <linux/device.h>
> > > > +#include <linux/io.h>
> > > > +#include <linux/mfd/syscon.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/of_graph.h>
> > > > +#include <linux/platform_device.h>
> > > > +
> > > > +#include "sun50i_planes.h"
> > > > +#include "sun8i_ui_layer.h"
> > > > +#include "sun8i_vi_layer.h"
> > > > +
> > > > +static bool sun50i_planes_node_is_planes(struct device_node *node)
> > > > +{
> > > > +       return !!of_match_node(sun50i_planes_of_table, node);
> > > > +}
> > > > +
> > > > +struct drm_plane **
> > > > +sun50i_planes_setup(struct device *dev, struct drm_device *drm,
> > > > +                   unsigned int mixer)
> > > > +{
> > > > +       struct sun50i_planes *planes = dev_get_drvdata(dev);
> > > > +       const struct sun50i_planes_quirks *quirks;
> > > > +       struct drm_plane **drm_planes;
> > > > +       const struct default_map *map;
> > > > +       unsigned int i;
> > > > +
> > > > +       if (!sun50i_planes_node_is_planes(dev->of_node)) {
> > > > +               dev_err(dev, "Device is not planes driver!\n");
> > > > +               return NULL;
> > > > +       }
> > > > +
> > > > +       if (!planes) {
> > > > +               dev_err(dev, "Planes driver is not loaded yet!\n");
> > > > +               return NULL;
> > > > +       }
> > > > +
> > > > +       if (mixer > 1) {
> > > > +               dev_err(dev, "Mixer index is too high!\n");
> > > > +               return NULL;
> > > > +       }
> > > > +
> > > > +       quirks = planes->quirks;
> > > > +       map = &quirks->def_map[mixer];
> > > > +
> > > > +       drm_planes = devm_kcalloc(drm->dev, map->num_ch + 1,
> > >
> > > Just a note: it seems we are missing the sentinel in sun8i_layers_init().
>
> Why do you think so? Current mainline code has mixer->cfg->vi_num +
> mixer->cfg->ui_num + 1.

I believe this was changed in your previous cleanups:

https://lore.kernel.org/all/20251104180942.61538-16-jernej.skrabec@gmail.com/

ChenYu

Re: [PATCH 5/7] drm/sun4i: Add planes driver

[Jernej Škrabec]

Dne četrtek, 25. december 2025 ob 20:30:23 Srednjeevropski standardni čas je Chen-Yu Tsai napisal(a):
> On Fri, Dec 26, 2025 at 3:17 AM Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
> >
> > Dne četrtek, 25. december 2025 ob 10:37:06 Srednjeevropski standardni čas je Chen-Yu Tsai napisal(a):
> > > On Thu, Dec 25, 2025 at 5:29 PM Chen-Yu Tsai <wens@kernel.org> wrote:
> > > >
> > > > On Sat, Nov 15, 2025 at 10:14 PM Jernej Skrabec
> > > > <jernej.skrabec@gmail.com> wrote:
> > > > >
> > > > > This driver serves just as planes sharing manager, needed for Display
> > > > > Engine 3.3 and newer.
> > > > >
> > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > > > > ---
> > > > >  drivers/gpu/drm/sun4i/Kconfig         |   8 +
> > > > >  drivers/gpu/drm/sun4i/Makefile        |   1 +
> > > > >  drivers/gpu/drm/sun4i/sun50i_planes.c | 205 ++++++++++++++++++++++++++
> > > > >  drivers/gpu/drm/sun4i/sun50i_planes.h |  43 ++++++
> > > > >  4 files changed, 257 insertions(+)
> > > > >  create mode 100644 drivers/gpu/drm/sun4i/sun50i_planes.c
> > > > >  create mode 100644 drivers/gpu/drm/sun4i/sun50i_planes.h
> > > > >
> > > > > diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
> > > > > index b56ba00aabca..946dd7606094 100644
> > > > > --- a/drivers/gpu/drm/sun4i/Kconfig
> > > > > +++ b/drivers/gpu/drm/sun4i/Kconfig
> > > > > @@ -85,4 +85,12 @@ config DRM_SUN8I_TCON_TOP
> > > > >           TCON TOP is responsible for configuring display pipeline for
> > > > >           HDMI, TVE and LCD.
> > > > >
> > > > > +config DRM_SUN50I_PLANES
> > > > > +       tristate
> > > > > +       default DRM_SUN4I if DRM_SUN8I_MIXER!=n
> > > > > +       help
> > > > > +         Chose this option if you have an Allwinner Soc with the
> > > > > +         Display Engine 3.3 or newer. Planes are shared resource
> > > > > +         between multiple mixers.
> > > > > +
> > > > >  endif
> > > > > diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> > > > > index bad7497a0d11..03f002abef15 100644
> > > > > --- a/drivers/gpu/drm/sun4i/Makefile
> > > > > +++ b/drivers/gpu/drm/sun4i/Makefile
> > > > > @@ -38,3 +38,4 @@ obj-$(CONFIG_DRM_SUN6I_DSI)   += sun6i_mipi_dsi.o
> > > > >  obj-$(CONFIG_DRM_SUN8I_DW_HDMI)        += sun8i-drm-hdmi.o
> > > > >  obj-$(CONFIG_DRM_SUN8I_MIXER)  += sun8i-mixer.o
> > > > >  obj-$(CONFIG_DRM_SUN8I_TCON_TOP) += sun8i_tcon_top.o
> > > > > +obj-$(CONFIG_DRM_SUN50I_PLANES)        += sun50i_planes.o
> > > >
> > > > I don't think you can have this as a separate module:
> > > >
> > > > a. You are using sun8i_vi_layer_init_one() and sun8i_ui_layer_init_one()
> > > >    from the sun8i-mixer module, and neither of them are exported symbols.
> > > >
> > > > b. You export sun50i_planes_setup() for sun8i-mixer to call, which ends
> > > >    up becoming a circular dependency.
> > > >
> > > > The easiest solution would be to just fold this into the sun8i-mixer module.
> >
> > I mimicked tcon-top module, but yeah, it's much less of a hassle to fold it
> > into sun8i-mixer.
> >
> > > >
> > > >
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun50i_planes.c b/drivers/gpu/drm/sun4i/sun50i_planes.c
> > > > > new file mode 100644
> > > > > index 000000000000..a99c01122990
> > > > > --- /dev/null
> > > > > +++ b/drivers/gpu/drm/sun4i/sun50i_planes.c
> > > > > @@ -0,0 +1,205 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > +/* Copyright (c) 2025 Jernej Skrabec <jernej.skrabec@gmail.com> */
> > > > > +
> > > > > +#include <linux/device.h>
> > > > > +#include <linux/io.h>
> > > > > +#include <linux/mfd/syscon.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/of.h>
> > > > > +#include <linux/of_graph.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +
> > > > > +#include "sun50i_planes.h"
> > > > > +#include "sun8i_ui_layer.h"
> > > > > +#include "sun8i_vi_layer.h"
> > > > > +
> > > > > +static bool sun50i_planes_node_is_planes(struct device_node *node)
> > > > > +{
> > > > > +       return !!of_match_node(sun50i_planes_of_table, node);
> > > > > +}
> > > > > +
> > > > > +struct drm_plane **
> > > > > +sun50i_planes_setup(struct device *dev, struct drm_device *drm,
> > > > > +                   unsigned int mixer)
> > > > > +{
> > > > > +       struct sun50i_planes *planes = dev_get_drvdata(dev);
> > > > > +       const struct sun50i_planes_quirks *quirks;
> > > > > +       struct drm_plane **drm_planes;
> > > > > +       const struct default_map *map;
> > > > > +       unsigned int i;
> > > > > +
> > > > > +       if (!sun50i_planes_node_is_planes(dev->of_node)) {
> > > > > +               dev_err(dev, "Device is not planes driver!\n");
> > > > > +               return NULL;
> > > > > +       }
> > > > > +
> > > > > +       if (!planes) {
> > > > > +               dev_err(dev, "Planes driver is not loaded yet!\n");
> > > > > +               return NULL;
> > > > > +       }
> > > > > +
> > > > > +       if (mixer > 1) {
> > > > > +               dev_err(dev, "Mixer index is too high!\n");
> > > > > +               return NULL;
> > > > > +       }
> > > > > +
> > > > > +       quirks = planes->quirks;
> > > > > +       map = &quirks->def_map[mixer];
> > > > > +
> > > > > +       drm_planes = devm_kcalloc(drm->dev, map->num_ch + 1,
> > > >
> > > > Just a note: it seems we are missing the sentinel in sun8i_layers_init().
> >
> > Why do you think so? Current mainline code has mixer->cfg->vi_num +
> > mixer->cfg->ui_num + 1.
> 
> I believe this was changed in your previous cleanups:
> 
> https://lore.kernel.org/all/20251104180942.61538-16-jernej.skrabec@gmail.com/

Ah, true. I'll send fix for -rc soon.

Best regards,
Jernej

[PATCH 6/7] dt-bindings: display: allwinner: Update H616 DE33 binding

[Jernej Skrabec]

As it turns out, current H616 DE33 binding was written based on
incomplete understanding of DE33 design. Namely, planes are shared
resource and not tied to specific mixer, which was the case for previous
generations of Display Engine (DE3 and earlier).

This means that current DE33 binding doesn't properly reflect HW and
using it would mean that second mixer (used for second display output)
can't be supported.

Update DE33 mixer binding so instead of referencing planes register
space, it contains phandle to newly introduced DE33 planes node.

There is no user of this binding yet, so changes can be made safely,
without breaking any backward compatibility.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 .../display/allwinner,sun8i-a83t-de2-mixer.yaml  | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/allwinner,sun8i-a83t-de2-mixer.yaml b/Documentation/devicetree/bindings/display/allwinner,sun8i-a83t-de2-mixer.yaml
index cbd18fd83e52..064e4ca7e419 100644
--- a/Documentation/devicetree/bindings/display/allwinner,sun8i-a83t-de2-mixer.yaml
+++ b/Documentation/devicetree/bindings/display/allwinner,sun8i-a83t-de2-mixer.yaml
@@ -46,6 +46,10 @@ properties:
   resets:
     maxItems: 1
 
+  allwinner,planes:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: Phandle of Display Engine 3.3 planes node
+
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
 
@@ -74,22 +78,22 @@ allOf:
       properties:
         reg:
           description: |
-            Registers for controlling individual layers of the display
-            engine (layers), global control (top), and display blending
-            control (display). Names are from Allwinner BSP kernel.
-          maxItems: 3
+            Registers for display blending control (display) and global
+            control (top). Names are from Allwinner BSP kernel.
+          maxItems: 2
         reg-names:
           items:
-            - const: layers
-            - const: top
             - const: display
+            - const: top
       required:
         - reg-names
+        - allwinner,planes
 
     else:
       properties:
         reg:
           maxItems: 1
+        allwinner,planes: false
 
 required:
   - compatible
-- 
2.51.2

Re: [PATCH 6/7] dt-bindings: display: allwinner: Update H616 DE33 binding

[Krzysztof Kozlowski]

On Sat, Nov 15, 2025 at 03:13:46PM +0100, Jernej Skrabec wrote:
> As it turns out, current H616 DE33 binding was written based on
> incomplete understanding of DE33 design. Namely, planes are shared
> resource and not tied to specific mixer, which was the case for previous
> generations of Display Engine (DE3 and earlier).
> 
> This means that current DE33 binding doesn't properly reflect HW and
> using it would mean that second mixer (used for second display output)
> can't be supported.
> 
> Update DE33 mixer binding so instead of referencing planes register
> space, it contains phandle to newly introduced DE33 planes node.
> 
> There is no user of this binding yet, so changes can be made safely,
> without breaking any backward compatibility.

And why would you configure statically - per soc - always the same plane
as per mixer? If you do that, it means it is really fixed and internal
to display engine thus should not be exposed in DT.

Describing each IP block resource in DT is way too granular.

Best regards,
Krzysztof

Re: [PATCH 6/7] dt-bindings: display: allwinner: Update H616 DE33 binding

[Krzysztof Kozlowski]

On 16/11/2025 12:33, Krzysztof Kozlowski wrote:
> On Sat, Nov 15, 2025 at 03:13:46PM +0100, Jernej Skrabec wrote:
>> As it turns out, current H616 DE33 binding was written based on
>> incomplete understanding of DE33 design. Namely, planes are shared
>> resource and not tied to specific mixer, which was the case for previous
>> generations of Display Engine (DE3 and earlier).
>>
>> This means that current DE33 binding doesn't properly reflect HW and
>> using it would mean that second mixer (used for second display output)
>> can't be supported.
>>
>> Update DE33 mixer binding so instead of referencing planes register
>> space, it contains phandle to newly introduced DE33 planes node.
>>
>> There is no user of this binding yet, so changes can be made safely,
>> without breaking any backward compatibility.
> 
> And why would you configure statically - per soc - always the same plane
> as per mixer? If you do that, it means it is really fixed and internal
> to display engine thus should not be exposed in DT.
> 
> Describing each IP block resource in DT is way too granular.
> 

BTW, everything is update, thus subject is really non-informative.

Best regards,
Krzysztof

Re: [PATCH 6/7] dt-bindings: display: allwinner: Update H616 DE33 binding

[Jernej Škrabec]

Hi!

Dne nedelja, 16. november 2025 ob 12:33:55 Srednjeevropski standardni čas je Krzysztof Kozlowski napisal(a):
> On 16/11/2025 12:33, Krzysztof Kozlowski wrote:
> > On Sat, Nov 15, 2025 at 03:13:46PM +0100, Jernej Skrabec wrote:
> >> As it turns out, current H616 DE33 binding was written based on
> >> incomplete understanding of DE33 design. Namely, planes are shared
> >> resource and not tied to specific mixer, which was the case for previous
> >> generations of Display Engine (DE3 and earlier).
> >>
> >> This means that current DE33 binding doesn't properly reflect HW and
> >> using it would mean that second mixer (used for second display output)
> >> can't be supported.
> >>
> >> Update DE33 mixer binding so instead of referencing planes register
> >> space, it contains phandle to newly introduced DE33 planes node.
> >>
> >> There is no user of this binding yet, so changes can be made safely,
> >> without breaking any backward compatibility.
> > 
> > And why would you configure statically - per soc - always the same plane
> > as per mixer? If you do that, it means it is really fixed and internal
> > to display engine thus should not be exposed in DT.

Not sure I understand what you mean. H616 SoC has 6 planes which are
represented with single DE33 planes node (see previous DT binding). 
Driver has to decide initial allocation. For example, 3 planes for each
mixer. However, nothing prevents to allocate 1 plane to first mixer and
5 to other. You can even allocate all 6 planes to one mixer and none to
the other, if board has only one output enabled.

In any case, plane allocation is runtime decision and has nothing to do
with DT. Since planes are shared resource, their register space can't be
assigned to only one mixer.

See [1] for example how this would look like.

> > 
> > Describing each IP block resource in DT is way too granular.
> > 
> 
> BTW, everything is update, thus subject is really non-informative.

I guess "fix" would be more descriptive.

Best regards,
Jernej

[1] https://github.com/jernejsk/linux-1/blob/d93d56d92db52c7ff228c0532a1045de02e0662c/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi#L181-L235

Re: [PATCH 6/7] dt-bindings: display: allwinner: Update H616 DE33 binding

[Chen-Yu Tsai]

On Sun, Nov 16, 2025 at 8:00 PM Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
>
> Hi!
>
> Dne nedelja, 16. november 2025 ob 12:33:55 Srednjeevropski standardni čas je Krzysztof Kozlowski napisal(a):
> > On 16/11/2025 12:33, Krzysztof Kozlowski wrote:
> > > On Sat, Nov 15, 2025 at 03:13:46PM +0100, Jernej Skrabec wrote:
> > >> As it turns out, current H616 DE33 binding was written based on
> > >> incomplete understanding of DE33 design. Namely, planes are shared
> > >> resource and not tied to specific mixer, which was the case for previous
> > >> generations of Display Engine (DE3 and earlier).
> > >>
> > >> This means that current DE33 binding doesn't properly reflect HW and
> > >> using it would mean that second mixer (used for second display output)
> > >> can't be supported.
> > >>
> > >> Update DE33 mixer binding so instead of referencing planes register
> > >> space, it contains phandle to newly introduced DE33 planes node.
> > >>
> > >> There is no user of this binding yet, so changes can be made safely,
> > >> without breaking any backward compatibility.
> > >
> > > And why would you configure statically - per soc - always the same plane
> > > as per mixer? If you do that, it means it is really fixed and internal
> > > to display engine thus should not be exposed in DT.
>
> Not sure I understand what you mean. H616 SoC has 6 planes which are
> represented with single DE33 planes node (see previous DT binding).
> Driver has to decide initial allocation. For example, 3 planes for each
> mixer. However, nothing prevents to allocate 1 plane to first mixer and
> 5 to other. You can even allocate all 6 planes to one mixer and none to
> the other, if board has only one output enabled.
>
> In any case, plane allocation is runtime decision and has nothing to do
> with DT. Since planes are shared resource, their register space can't be
> assigned to only one mixer.
>
> See [1] for example how this would look like.
>
> > >
> > > Describing each IP block resource in DT is way too granular.
> > >
> >
> > BTW, everything is update, thus subject is really non-informative.
>
> I guess "fix" would be more descriptive.

Or maybe be more specific, like "split out layers register space to
separate binding / node".


ChenYu

> Best regards,
> Jernej
>
> [1] https://github.com/jernejsk/linux-1/blob/d93d56d92db52c7ff228c0532a1045de02e0662c/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi#L181-L235
>
>
>
>

[PATCH 7/7] drm/sun4i: switch DE33 to new bindings

[Jernej Skrabec]

Now that everything is in place, switch DE33 to new bindings.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/gpu/drm/sun4i/sun8i_mixer.c | 130 +++++++++++++++-------------
 drivers/gpu/drm/sun4i/sun8i_mixer.h |  10 +--
 2 files changed, 71 insertions(+), 69 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
index fde3b677e925..da213e54e653 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
@@ -13,6 +13,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_graph.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/reset.h>
 
@@ -24,6 +25,7 @@
 #include <drm/drm_probe_helper.h>
 
 #include "sun4i_drv.h"
+#include "sun50i_planes.h"
 #include "sun8i_mixer.h"
 #include "sun8i_ui_layer.h"
 #include "sun8i_vi_layer.h"
@@ -256,7 +258,6 @@ static void sun8i_mixer_commit(struct sunxi_engine *engine,
 {
 	struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
 	u32 bld_base = sun8i_blender_base(mixer);
-	struct regmap *bld_regs = sun8i_blender_regmap(mixer);
 	struct drm_plane_state *plane_state;
 	struct drm_plane *plane;
 	u32 route = 0, pipe_en = 0;
@@ -293,16 +294,16 @@ static void sun8i_mixer_commit(struct sunxi_engine *engine,
 		route |= layer->index << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
 		pipe_en |= SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
 
-		regmap_write(bld_regs,
+		regmap_write(engine->regs,
 			     SUN8I_MIXER_BLEND_ATTR_COORD(bld_base, zpos),
 			     SUN8I_MIXER_COORD(x, y));
-		regmap_write(bld_regs,
+		regmap_write(engine->regs,
 			     SUN8I_MIXER_BLEND_ATTR_INSIZE(bld_base, zpos),
 			     SUN8I_MIXER_SIZE(w, h));
 	}
 
-	regmap_write(bld_regs, SUN8I_MIXER_BLEND_ROUTE(bld_base), route);
-	regmap_write(bld_regs, SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
+	regmap_write(engine->regs, SUN8I_MIXER_BLEND_ROUTE(bld_base), route);
+	regmap_write(engine->regs, SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
 		     pipe_en | SUN8I_MIXER_BLEND_PIPE_CTL_FC_EN(0));
 
 	if (mixer->cfg->de_type != SUN8I_MIXER_DE33)
@@ -317,7 +318,6 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
 	struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
 	int plane_cnt = mixer->cfg->ui_num + mixer->cfg->vi_num;
 	enum drm_plane_type type;
-	unsigned int phy_index;
 	int i;
 
 	planes = devm_kcalloc(drm->dev, plane_cnt, sizeof(*planes), GFP_KERNEL);
@@ -332,12 +332,8 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
 		else
 			type = DRM_PLANE_TYPE_OVERLAY;
 
-		phy_index = i;
-		if (mixer->cfg->de_type == SUN8I_MIXER_DE33)
-			phy_index = mixer->cfg->map[i];
-
 		layer = sun8i_vi_layer_init_one(drm, type, mixer->engine.regs,
-						i, phy_index, plane_cnt,
+						i, i, plane_cnt,
 						&mixer->cfg->lay_cfg);
 		if (IS_ERR(layer)) {
 			dev_err(drm->dev,
@@ -357,12 +353,8 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
 		else
 			type = DRM_PLANE_TYPE_OVERLAY;
 
-		phy_index = index;
-		if (mixer->cfg->de_type == SUN8I_MIXER_DE33)
-			phy_index = mixer->cfg->map[index];
-
 		layer = sun8i_ui_layer_init_one(drm, type, mixer->engine.regs,
-						index, phy_index, plane_cnt,
+						index, index, plane_cnt,
 						&mixer->cfg->lay_cfg);
 		if (IS_ERR(layer)) {
 			dev_err(drm->dev, "Couldn't initialize %s plane\n",
@@ -376,16 +368,25 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
 	return planes;
 }
 
+static struct drm_plane **sun50i_layers_init(struct drm_device *drm,
+					     struct sunxi_engine *engine)
+{
+	struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
+
+	if (IS_ENABLED(CONFIG_DRM_SUN50I_PLANES))
+		return sun50i_planes_setup(mixer->planes_dev, drm, engine->id);
+
+	return NULL;
+}
+
 static void sun8i_mixer_mode_set(struct sunxi_engine *engine,
 				 const struct drm_display_mode *mode)
 {
 	struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
-	struct regmap *bld_regs;
 	u32 bld_base, size, val;
 	bool interlaced;
 
 	bld_base = sun8i_blender_base(mixer);
-	bld_regs = sun8i_blender_regmap(mixer);
 	interlaced = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
 	size = SUN8I_MIXER_SIZE(mode->hdisplay, mode->vdisplay);
 
@@ -397,14 +398,14 @@ static void sun8i_mixer_mode_set(struct sunxi_engine *engine,
 	else
 		regmap_write(mixer->engine.regs, SUN8I_MIXER_GLOBAL_SIZE, size);
 
-	regmap_write(bld_regs, SUN8I_MIXER_BLEND_OUTSIZE(bld_base), size);
+	regmap_write(engine->regs, SUN8I_MIXER_BLEND_OUTSIZE(bld_base), size);
 
 	if (interlaced)
 		val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;
 	else
 		val = 0;
 
-	regmap_update_bits(bld_regs, SUN8I_MIXER_BLEND_OUTCTL(bld_base),
+	regmap_update_bits(engine->regs, SUN8I_MIXER_BLEND_OUTCTL(bld_base),
 			   SUN8I_MIXER_BLEND_OUTCTL_INTERLACED, val);
 
 	DRM_DEBUG_DRIVER("Switching display mixer interlaced mode %s\n",
@@ -417,8 +418,14 @@ static const struct sunxi_engine_ops sun8i_engine_ops = {
 	.mode_set	= sun8i_mixer_mode_set,
 };
 
+static const struct sunxi_engine_ops sun50i_engine_ops = {
+	.commit		= sun8i_mixer_commit,
+	.layers_init	= sun50i_layers_init,
+	.mode_set	= sun8i_mixer_mode_set,
+};
+
 static const struct regmap_config sun8i_mixer_regmap_config = {
-	.name		= "layers",
+	.name		= "display",
 	.reg_bits	= 32,
 	.val_bits	= 32,
 	.reg_stride	= 4,
@@ -433,14 +440,6 @@ static const struct regmap_config sun8i_top_regmap_config = {
 	.max_register	= 0x3c,
 };
 
-static const struct regmap_config sun8i_disp_regmap_config = {
-	.name		= "display",
-	.reg_bits	= 32,
-	.val_bits	= 32,
-	.reg_stride	= 4,
-	.max_register	= 0x20000,
-};
-
 static int sun8i_mixer_of_get_id(struct device_node *node)
 {
 	struct device_node *ep, *remote;
@@ -463,17 +462,14 @@ static int sun8i_mixer_of_get_id(struct device_node *node)
 
 static void sun8i_mixer_init(struct sun8i_mixer *mixer)
 {
-	struct regmap *top_regs, *disp_regs;
 	unsigned int base = sun8i_blender_base(mixer);
+	struct regmap *top_regs;
 	int plane_cnt, i;
 
-	if (mixer->cfg->de_type == SUN8I_MIXER_DE33) {
+	if (mixer->cfg->de_type == SUN8I_MIXER_DE33)
 		top_regs = mixer->top_regs;
-		disp_regs = mixer->disp_regs;
-	} else {
+	else
 		top_regs = mixer->engine.regs;
-		disp_regs = mixer->engine.regs;
-	}
 
 	/* Enable the mixer */
 	regmap_write(top_regs, SUN8I_MIXER_GLOBAL_CTL,
@@ -483,25 +479,25 @@ static void sun8i_mixer_init(struct sun8i_mixer *mixer)
 		regmap_write(top_regs, SUN50I_MIXER_GLOBAL_CLK, 1);
 
 	/* Set background color to black */
-	regmap_write(disp_regs, SUN8I_MIXER_BLEND_BKCOLOR(base),
+	regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_BKCOLOR(base),
 		     SUN8I_MIXER_BLEND_COLOR_BLACK);
 
 	/*
 	 * Set fill color of bottom plane to black. Generally not needed
 	 * except when VI plane is at bottom (zpos = 0) and enabled.
 	 */
-	regmap_write(disp_regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
+	regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
 		     SUN8I_MIXER_BLEND_PIPE_CTL_FC_EN(0));
-	regmap_write(disp_regs, SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, 0),
+	regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, 0),
 		     SUN8I_MIXER_BLEND_COLOR_BLACK);
 
 	plane_cnt = mixer->cfg->vi_num + mixer->cfg->ui_num;
 	for (i = 0; i < plane_cnt; i++)
-		regmap_write(disp_regs,
+		regmap_write(mixer->engine.regs,
 			     SUN8I_MIXER_BLEND_MODE(base, i),
 			     SUN8I_MIXER_BLEND_MODE_DEF);
 
-	regmap_update_bits(disp_regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
+	regmap_update_bits(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
 			   SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, 0);
 }
 
@@ -532,7 +528,6 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
 	if (!mixer)
 		return -ENOMEM;
 	dev_set_drvdata(dev, mixer);
-	mixer->engine.ops = &sun8i_engine_ops;
 	mixer->engine.node = dev->of_node;
 
 	if (of_property_present(dev->of_node, "iommus")) {
@@ -562,6 +557,11 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
 	if (!mixer->cfg)
 		return -EINVAL;
 
+	if (mixer->cfg->de_type == SUN8I_MIXER_DE33)
+		mixer->engine.ops = &sun50i_engine_ops;
+	else
+		mixer->engine.ops = &sun8i_engine_ops;
+
 	regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(regs))
 		return PTR_ERR(regs);
@@ -584,17 +584,6 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
 			dev_err(dev, "Couldn't create the top regmap\n");
 			return PTR_ERR(mixer->top_regs);
 		}
-
-		regs = devm_platform_ioremap_resource_byname(pdev, "display");
-		if (IS_ERR(regs))
-			return PTR_ERR(regs);
-
-		mixer->disp_regs = devm_regmap_init_mmio(dev, regs,
-							 &sun8i_disp_regmap_config);
-		if (IS_ERR(mixer->disp_regs)) {
-			dev_err(dev, "Couldn't create the disp regmap\n");
-			return PTR_ERR(mixer->disp_regs);
-		}
 	}
 
 	mixer->reset = devm_reset_control_get(dev, NULL);
@@ -634,6 +623,33 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
 
 	clk_prepare_enable(mixer->mod_clk);
 
+	if (mixer->cfg->de_type == SUN8I_MIXER_DE33) {
+		struct platform_device *pdev;
+		struct device_node *np;
+		void *data;
+
+		np = of_parse_phandle(dev->of_node, "allwinner,planes", 0);
+		if (!np) {
+			ret = -ENODEV;
+			goto err_disable_mod_clk;
+		}
+
+		pdev = of_find_device_by_node(np);
+		of_node_put(np);
+		if (!pdev) {
+			ret = -EPROBE_DEFER;
+			goto err_disable_mod_clk;
+		}
+
+		data = platform_get_drvdata(pdev);
+		if (!data) {
+			put_device(&pdev->dev);
+			return -EPROBE_DEFER;
+		}
+
+		mixer->planes_dev = &pdev->dev;
+	}
+
 	list_add_tail(&mixer->engine.list, &drv->engine_list);
 
 	/* Reset registers and disable unused sub-engines */
@@ -668,6 +684,8 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
 
 	return 0;
 
+err_disable_mod_clk:
+	clk_disable_unprepare(mixer->mod_clk);
 err_disable_bus_clk:
 	clk_disable_unprepare(mixer->bus_clk);
 err_assert_reset:
@@ -863,16 +881,8 @@ static const struct sun8i_mixer_cfg sun50i_h6_mixer0_cfg = {
 };
 
 static const struct sun8i_mixer_cfg sun50i_h616_mixer0_cfg = {
-	.lay_cfg = {
-		.de_type	= SUN8I_MIXER_DE33,
-		.scaler_mask	= 0xf,
-		.scanline_yuv	= 4096,
-	},
 	.de_type	= SUN8I_MIXER_DE33,
 	.mod_rate	= 600000000,
-	.ui_num		= 3,
-	.vi_num		= 1,
-	.map		= {0, 6, 7, 8},
 };
 
 static const struct of_device_id sun8i_mixer_of_table[] = {
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h
index e2f83301aae8..7abc88c898d9 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
@@ -202,7 +202,6 @@ struct sun8i_mixer_cfg {
 	int			ui_num;
 	unsigned int		de_type;
 	unsigned long		mod_rate;
-	unsigned int		map[6];
 };
 
 struct sun8i_mixer {
@@ -216,7 +215,7 @@ struct sun8i_mixer {
 	struct clk			*mod_clk;
 
 	struct regmap			*top_regs;
-	struct regmap			*disp_regs;
+	struct device			*planes_dev;
 };
 
 enum {
@@ -252,13 +251,6 @@ sun8i_blender_base(struct sun8i_mixer *mixer)
 	return mixer->cfg->de_type == SUN8I_MIXER_DE3 ? DE3_BLD_BASE : DE2_BLD_BASE;
 }
 
-static inline struct regmap *
-sun8i_blender_regmap(struct sun8i_mixer *mixer)
-{
-	return mixer->cfg->de_type == SUN8I_MIXER_DE33 ?
-		mixer->disp_regs : mixer->engine.regs;
-}
-
 static inline u32
 sun8i_channel_base(struct sun8i_layer *layer)
 {
-- 
2.51.2

Re: [PATCH 7/7] drm/sun4i: switch DE33 to new bindings

[Chen-Yu Tsai]

On Sat, Nov 15, 2025 at 10:14 PM Jernej Skrabec
<jernej.skrabec@gmail.com> wrote:
>
> Now that everything is in place, switch DE33 to new bindings.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/gpu/drm/sun4i/sun8i_mixer.c | 130 +++++++++++++++-------------
>  drivers/gpu/drm/sun4i/sun8i_mixer.h |  10 +--
>  2 files changed, 71 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> index fde3b677e925..da213e54e653 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -13,6 +13,7 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/of_graph.h>
> +#include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/reset.h>
>
> @@ -24,6 +25,7 @@
>  #include <drm/drm_probe_helper.h>
>
>  #include "sun4i_drv.h"
> +#include "sun50i_planes.h"
>  #include "sun8i_mixer.h"
>  #include "sun8i_ui_layer.h"
>  #include "sun8i_vi_layer.h"
> @@ -256,7 +258,6 @@ static void sun8i_mixer_commit(struct sunxi_engine *engine,
>  {
>         struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
>         u32 bld_base = sun8i_blender_base(mixer);
> -       struct regmap *bld_regs = sun8i_blender_regmap(mixer);
>         struct drm_plane_state *plane_state;
>         struct drm_plane *plane;
>         u32 route = 0, pipe_en = 0;
> @@ -293,16 +294,16 @@ static void sun8i_mixer_commit(struct sunxi_engine *engine,
>                 route |= layer->index << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
>                 pipe_en |= SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
>
> -               regmap_write(bld_regs,
> +               regmap_write(engine->regs,
>                              SUN8I_MIXER_BLEND_ATTR_COORD(bld_base, zpos),
>                              SUN8I_MIXER_COORD(x, y));
> -               regmap_write(bld_regs,
> +               regmap_write(engine->regs,
>                              SUN8I_MIXER_BLEND_ATTR_INSIZE(bld_base, zpos),
>                              SUN8I_MIXER_SIZE(w, h));
>         }
>
> -       regmap_write(bld_regs, SUN8I_MIXER_BLEND_ROUTE(bld_base), route);
> -       regmap_write(bld_regs, SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> +       regmap_write(engine->regs, SUN8I_MIXER_BLEND_ROUTE(bld_base), route);
> +       regmap_write(engine->regs, SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
>                      pipe_en | SUN8I_MIXER_BLEND_PIPE_CTL_FC_EN(0));
>
>         if (mixer->cfg->de_type != SUN8I_MIXER_DE33)
> @@ -317,7 +318,6 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
>         struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
>         int plane_cnt = mixer->cfg->ui_num + mixer->cfg->vi_num;
>         enum drm_plane_type type;
> -       unsigned int phy_index;
>         int i;
>
>         planes = devm_kcalloc(drm->dev, plane_cnt, sizeof(*planes), GFP_KERNEL);
> @@ -332,12 +332,8 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
>                 else
>                         type = DRM_PLANE_TYPE_OVERLAY;
>
> -               phy_index = i;
> -               if (mixer->cfg->de_type == SUN8I_MIXER_DE33)
> -                       phy_index = mixer->cfg->map[i];
> -
>                 layer = sun8i_vi_layer_init_one(drm, type, mixer->engine.regs,
> -                                               i, phy_index, plane_cnt,
> +                                               i, i, plane_cnt,
>                                                 &mixer->cfg->lay_cfg);
>                 if (IS_ERR(layer)) {
>                         dev_err(drm->dev,
> @@ -357,12 +353,8 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
>                 else
>                         type = DRM_PLANE_TYPE_OVERLAY;
>
> -               phy_index = index;
> -               if (mixer->cfg->de_type == SUN8I_MIXER_DE33)
> -                       phy_index = mixer->cfg->map[index];
> -
>                 layer = sun8i_ui_layer_init_one(drm, type, mixer->engine.regs,
> -                                               index, phy_index, plane_cnt,
> +                                               index, index, plane_cnt,
>                                                 &mixer->cfg->lay_cfg);
>                 if (IS_ERR(layer)) {
>                         dev_err(drm->dev, "Couldn't initialize %s plane\n",
> @@ -376,16 +368,25 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
>         return planes;
>  }
>
> +static struct drm_plane **sun50i_layers_init(struct drm_device *drm,
> +                                            struct sunxi_engine *engine)
> +{
> +       struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> +
> +       if (IS_ENABLED(CONFIG_DRM_SUN50I_PLANES))
> +               return sun50i_planes_setup(mixer->planes_dev, drm, engine->id);
> +
> +       return NULL;
> +}
> +
>  static void sun8i_mixer_mode_set(struct sunxi_engine *engine,
>                                  const struct drm_display_mode *mode)
>  {
>         struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> -       struct regmap *bld_regs;
>         u32 bld_base, size, val;
>         bool interlaced;
>
>         bld_base = sun8i_blender_base(mixer);
> -       bld_regs = sun8i_blender_regmap(mixer);
>         interlaced = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
>         size = SUN8I_MIXER_SIZE(mode->hdisplay, mode->vdisplay);
>
> @@ -397,14 +398,14 @@ static void sun8i_mixer_mode_set(struct sunxi_engine *engine,
>         else
>                 regmap_write(mixer->engine.regs, SUN8I_MIXER_GLOBAL_SIZE, size);
>
> -       regmap_write(bld_regs, SUN8I_MIXER_BLEND_OUTSIZE(bld_base), size);
> +       regmap_write(engine->regs, SUN8I_MIXER_BLEND_OUTSIZE(bld_base), size);
>
>         if (interlaced)
>                 val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;
>         else
>                 val = 0;
>
> -       regmap_update_bits(bld_regs, SUN8I_MIXER_BLEND_OUTCTL(bld_base),
> +       regmap_update_bits(engine->regs, SUN8I_MIXER_BLEND_OUTCTL(bld_base),
>                            SUN8I_MIXER_BLEND_OUTCTL_INTERLACED, val);
>
>         DRM_DEBUG_DRIVER("Switching display mixer interlaced mode %s\n",
> @@ -417,8 +418,14 @@ static const struct sunxi_engine_ops sun8i_engine_ops = {
>         .mode_set       = sun8i_mixer_mode_set,
>  };
>
> +static const struct sunxi_engine_ops sun50i_engine_ops = {
> +       .commit         = sun8i_mixer_commit,
> +       .layers_init    = sun50i_layers_init,
> +       .mode_set       = sun8i_mixer_mode_set,
> +};
> +
>  static const struct regmap_config sun8i_mixer_regmap_config = {
> -       .name           = "layers",
> +       .name           = "display",
>         .reg_bits       = 32,
>         .val_bits       = 32,
>         .reg_stride     = 4,
> @@ -433,14 +440,6 @@ static const struct regmap_config sun8i_top_regmap_config = {
>         .max_register   = 0x3c,
>  };
>
> -static const struct regmap_config sun8i_disp_regmap_config = {
> -       .name           = "display",
> -       .reg_bits       = 32,
> -       .val_bits       = 32,
> -       .reg_stride     = 4,
> -       .max_register   = 0x20000,
> -};
> -
>  static int sun8i_mixer_of_get_id(struct device_node *node)
>  {
>         struct device_node *ep, *remote;
> @@ -463,17 +462,14 @@ static int sun8i_mixer_of_get_id(struct device_node *node)
>
>  static void sun8i_mixer_init(struct sun8i_mixer *mixer)
>  {
> -       struct regmap *top_regs, *disp_regs;
>         unsigned int base = sun8i_blender_base(mixer);
> +       struct regmap *top_regs;
>         int plane_cnt, i;
>
> -       if (mixer->cfg->de_type == SUN8I_MIXER_DE33) {
> +       if (mixer->cfg->de_type == SUN8I_MIXER_DE33)
>                 top_regs = mixer->top_regs;
> -               disp_regs = mixer->disp_regs;
> -       } else {
> +       else
>                 top_regs = mixer->engine.regs;
> -               disp_regs = mixer->engine.regs;
> -       }
>
>         /* Enable the mixer */
>         regmap_write(top_regs, SUN8I_MIXER_GLOBAL_CTL,
> @@ -483,25 +479,25 @@ static void sun8i_mixer_init(struct sun8i_mixer *mixer)
>                 regmap_write(top_regs, SUN50I_MIXER_GLOBAL_CLK, 1);
>
>         /* Set background color to black */
> -       regmap_write(disp_regs, SUN8I_MIXER_BLEND_BKCOLOR(base),
> +       regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_BKCOLOR(base),
>                      SUN8I_MIXER_BLEND_COLOR_BLACK);
>
>         /*
>          * Set fill color of bottom plane to black. Generally not needed
>          * except when VI plane is at bottom (zpos = 0) and enabled.
>          */
> -       regmap_write(disp_regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
> +       regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
>                      SUN8I_MIXER_BLEND_PIPE_CTL_FC_EN(0));
> -       regmap_write(disp_regs, SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, 0),
> +       regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, 0),
>                      SUN8I_MIXER_BLEND_COLOR_BLACK);
>
>         plane_cnt = mixer->cfg->vi_num + mixer->cfg->ui_num;
>         for (i = 0; i < plane_cnt; i++)
> -               regmap_write(disp_regs,
> +               regmap_write(mixer->engine.regs,
>                              SUN8I_MIXER_BLEND_MODE(base, i),
>                              SUN8I_MIXER_BLEND_MODE_DEF);
>
> -       regmap_update_bits(disp_regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
> +       regmap_update_bits(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
>                            SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, 0);
>  }
>
> @@ -532,7 +528,6 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
>         if (!mixer)
>                 return -ENOMEM;
>         dev_set_drvdata(dev, mixer);
> -       mixer->engine.ops = &sun8i_engine_ops;
>         mixer->engine.node = dev->of_node;
>
>         if (of_property_present(dev->of_node, "iommus")) {
> @@ -562,6 +557,11 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
>         if (!mixer->cfg)
>                 return -EINVAL;
>
> +       if (mixer->cfg->de_type == SUN8I_MIXER_DE33)
> +               mixer->engine.ops = &sun50i_engine_ops;

You're missing an IS_ENABLED() clause here if you wanted to make the DE 3.3
planes driver optional. Though as I mentioned in the other patch, splittig
the two modules might not work.

> +       else
> +               mixer->engine.ops = &sun8i_engine_ops;
> +
>         regs = devm_platform_ioremap_resource(pdev, 0);
>         if (IS_ERR(regs))
>                 return PTR_ERR(regs);
> @@ -584,17 +584,6 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
>                         dev_err(dev, "Couldn't create the top regmap\n");
>                         return PTR_ERR(mixer->top_regs);
>                 }
> -
> -               regs = devm_platform_ioremap_resource_byname(pdev, "display");
> -               if (IS_ERR(regs))
> -                       return PTR_ERR(regs);
> -
> -               mixer->disp_regs = devm_regmap_init_mmio(dev, regs,
> -                                                        &sun8i_disp_regmap_config);
> -               if (IS_ERR(mixer->disp_regs)) {
> -                       dev_err(dev, "Couldn't create the disp regmap\n");
> -                       return PTR_ERR(mixer->disp_regs);
> -               }
>         }
>
>         mixer->reset = devm_reset_control_get(dev, NULL);
> @@ -634,6 +623,33 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
>
>         clk_prepare_enable(mixer->mod_clk);
>
> +       if (mixer->cfg->de_type == SUN8I_MIXER_DE33) {
> +               struct platform_device *pdev;
> +               struct device_node *np;
> +               void *data;
> +
> +               np = of_parse_phandle(dev->of_node, "allwinner,planes", 0);
> +               if (!np) {
> +                       ret = -ENODEV;
> +                       goto err_disable_mod_clk;
> +               }
> +
> +               pdev = of_find_device_by_node(np);

You need to add a matching put_device() in the unbind function.

Side note:

This bind function is using a lot of devm_ functions. These have the wrong
lifetime. I think it would be better if we could move resource acquisition
into the probe function.


> +               of_node_put(np);
> +               if (!pdev) {
> +                       ret = -EPROBE_DEFER;
> +                       goto err_disable_mod_clk;
> +               }
> +
> +               data = platform_get_drvdata(pdev);
> +               if (!data) {
> +                       put_device(&pdev->dev);
> +                       return -EPROBE_DEFER;

Should be a goto here?


ChenYu

> +               }
> +
> +               mixer->planes_dev = &pdev->dev;
> +       }
> +
>         list_add_tail(&mixer->engine.list, &drv->engine_list);
>
>         /* Reset registers and disable unused sub-engines */
> @@ -668,6 +684,8 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
>
>         return 0;
>
> +err_disable_mod_clk:
> +       clk_disable_unprepare(mixer->mod_clk);
>  err_disable_bus_clk:
>         clk_disable_unprepare(mixer->bus_clk);
>  err_assert_reset:
> @@ -863,16 +881,8 @@ static const struct sun8i_mixer_cfg sun50i_h6_mixer0_cfg = {
>  };
>
>  static const struct sun8i_mixer_cfg sun50i_h616_mixer0_cfg = {
> -       .lay_cfg = {
> -               .de_type        = SUN8I_MIXER_DE33,
> -               .scaler_mask    = 0xf,
> -               .scanline_yuv   = 4096,
> -       },
>         .de_type        = SUN8I_MIXER_DE33,
>         .mod_rate       = 600000000,
> -       .ui_num         = 3,
> -       .vi_num         = 1,
> -       .map            = {0, 6, 7, 8},
>  };
>
>  static const struct of_device_id sun8i_mixer_of_table[] = {
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> index e2f83301aae8..7abc88c898d9 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> @@ -202,7 +202,6 @@ struct sun8i_mixer_cfg {
>         int                     ui_num;
>         unsigned int            de_type;
>         unsigned long           mod_rate;
> -       unsigned int            map[6];
>  };
>
>  struct sun8i_mixer {
> @@ -216,7 +215,7 @@ struct sun8i_mixer {
>         struct clk                      *mod_clk;
>
>         struct regmap                   *top_regs;
> -       struct regmap                   *disp_regs;
> +       struct device                   *planes_dev;
>  };
>
>  enum {
> @@ -252,13 +251,6 @@ sun8i_blender_base(struct sun8i_mixer *mixer)
>         return mixer->cfg->de_type == SUN8I_MIXER_DE3 ? DE3_BLD_BASE : DE2_BLD_BASE;
>  }
>
> -static inline struct regmap *
> -sun8i_blender_regmap(struct sun8i_mixer *mixer)
> -{
> -       return mixer->cfg->de_type == SUN8I_MIXER_DE33 ?
> -               mixer->disp_regs : mixer->engine.regs;
> -}
> -
>  static inline u32
>  sun8i_channel_base(struct sun8i_layer *layer)
>  {
> --
> 2.51.2
>
>

Re: [PATCH 7/7] drm/sun4i: switch DE33 to new bindings

[Jernej Škrabec]

Dne četrtek, 25. december 2025 ob 10:49:47 Srednjeevropski standardni čas je Chen-Yu Tsai napisal(a):
> On Sat, Nov 15, 2025 at 10:14 PM Jernej Skrabec
> <jernej.skrabec@gmail.com> wrote:
> >
> > Now that everything is in place, switch DE33 to new bindings.
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > ---
> >  drivers/gpu/drm/sun4i/sun8i_mixer.c | 130 +++++++++++++++-------------
> >  drivers/gpu/drm/sun4i/sun8i_mixer.h |  10 +--
> >  2 files changed, 71 insertions(+), 69 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > index fde3b677e925..da213e54e653 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> >  #include <linux/of_graph.h>
> > +#include <linux/of_platform.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/reset.h>
> >
> > @@ -24,6 +25,7 @@
> >  #include <drm/drm_probe_helper.h>
> >
> >  #include "sun4i_drv.h"
> > +#include "sun50i_planes.h"
> >  #include "sun8i_mixer.h"
> >  #include "sun8i_ui_layer.h"
> >  #include "sun8i_vi_layer.h"
> > @@ -256,7 +258,6 @@ static void sun8i_mixer_commit(struct sunxi_engine *engine,
> >  {
> >         struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> >         u32 bld_base = sun8i_blender_base(mixer);
> > -       struct regmap *bld_regs = sun8i_blender_regmap(mixer);
> >         struct drm_plane_state *plane_state;
> >         struct drm_plane *plane;
> >         u32 route = 0, pipe_en = 0;
> > @@ -293,16 +294,16 @@ static void sun8i_mixer_commit(struct sunxi_engine *engine,
> >                 route |= layer->index << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
> >                 pipe_en |= SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
> >
> > -               regmap_write(bld_regs,
> > +               regmap_write(engine->regs,
> >                              SUN8I_MIXER_BLEND_ATTR_COORD(bld_base, zpos),
> >                              SUN8I_MIXER_COORD(x, y));
> > -               regmap_write(bld_regs,
> > +               regmap_write(engine->regs,
> >                              SUN8I_MIXER_BLEND_ATTR_INSIZE(bld_base, zpos),
> >                              SUN8I_MIXER_SIZE(w, h));
> >         }
> >
> > -       regmap_write(bld_regs, SUN8I_MIXER_BLEND_ROUTE(bld_base), route);
> > -       regmap_write(bld_regs, SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> > +       regmap_write(engine->regs, SUN8I_MIXER_BLEND_ROUTE(bld_base), route);
> > +       regmap_write(engine->regs, SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> >                      pipe_en | SUN8I_MIXER_BLEND_PIPE_CTL_FC_EN(0));
> >
> >         if (mixer->cfg->de_type != SUN8I_MIXER_DE33)
> > @@ -317,7 +318,6 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
> >         struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> >         int plane_cnt = mixer->cfg->ui_num + mixer->cfg->vi_num;
> >         enum drm_plane_type type;
> > -       unsigned int phy_index;
> >         int i;
> >
> >         planes = devm_kcalloc(drm->dev, plane_cnt, sizeof(*planes), GFP_KERNEL);
> > @@ -332,12 +332,8 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
> >                 else
> >                         type = DRM_PLANE_TYPE_OVERLAY;
> >
> > -               phy_index = i;
> > -               if (mixer->cfg->de_type == SUN8I_MIXER_DE33)
> > -                       phy_index = mixer->cfg->map[i];
> > -
> >                 layer = sun8i_vi_layer_init_one(drm, type, mixer->engine.regs,
> > -                                               i, phy_index, plane_cnt,
> > +                                               i, i, plane_cnt,
> >                                                 &mixer->cfg->lay_cfg);
> >                 if (IS_ERR(layer)) {
> >                         dev_err(drm->dev,
> > @@ -357,12 +353,8 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
> >                 else
> >                         type = DRM_PLANE_TYPE_OVERLAY;
> >
> > -               phy_index = index;
> > -               if (mixer->cfg->de_type == SUN8I_MIXER_DE33)
> > -                       phy_index = mixer->cfg->map[index];
> > -
> >                 layer = sun8i_ui_layer_init_one(drm, type, mixer->engine.regs,
> > -                                               index, phy_index, plane_cnt,
> > +                                               index, index, plane_cnt,
> >                                                 &mixer->cfg->lay_cfg);
> >                 if (IS_ERR(layer)) {
> >                         dev_err(drm->dev, "Couldn't initialize %s plane\n",
> > @@ -376,16 +368,25 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
> >         return planes;
> >  }
> >
> > +static struct drm_plane **sun50i_layers_init(struct drm_device *drm,
> > +                                            struct sunxi_engine *engine)
> > +{
> > +       struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> > +
> > +       if (IS_ENABLED(CONFIG_DRM_SUN50I_PLANES))
> > +               return sun50i_planes_setup(mixer->planes_dev, drm, engine->id);
> > +
> > +       return NULL;
> > +}
> > +
> >  static void sun8i_mixer_mode_set(struct sunxi_engine *engine,
> >                                  const struct drm_display_mode *mode)
> >  {
> >         struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> > -       struct regmap *bld_regs;
> >         u32 bld_base, size, val;
> >         bool interlaced;
> >
> >         bld_base = sun8i_blender_base(mixer);
> > -       bld_regs = sun8i_blender_regmap(mixer);
> >         interlaced = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
> >         size = SUN8I_MIXER_SIZE(mode->hdisplay, mode->vdisplay);
> >
> > @@ -397,14 +398,14 @@ static void sun8i_mixer_mode_set(struct sunxi_engine *engine,
> >         else
> >                 regmap_write(mixer->engine.regs, SUN8I_MIXER_GLOBAL_SIZE, size);
> >
> > -       regmap_write(bld_regs, SUN8I_MIXER_BLEND_OUTSIZE(bld_base), size);
> > +       regmap_write(engine->regs, SUN8I_MIXER_BLEND_OUTSIZE(bld_base), size);
> >
> >         if (interlaced)
> >                 val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;
> >         else
> >                 val = 0;
> >
> > -       regmap_update_bits(bld_regs, SUN8I_MIXER_BLEND_OUTCTL(bld_base),
> > +       regmap_update_bits(engine->regs, SUN8I_MIXER_BLEND_OUTCTL(bld_base),
> >                            SUN8I_MIXER_BLEND_OUTCTL_INTERLACED, val);
> >
> >         DRM_DEBUG_DRIVER("Switching display mixer interlaced mode %s\n",
> > @@ -417,8 +418,14 @@ static const struct sunxi_engine_ops sun8i_engine_ops = {
> >         .mode_set       = sun8i_mixer_mode_set,
> >  };
> >
> > +static const struct sunxi_engine_ops sun50i_engine_ops = {
> > +       .commit         = sun8i_mixer_commit,
> > +       .layers_init    = sun50i_layers_init,
> > +       .mode_set       = sun8i_mixer_mode_set,
> > +};
> > +
> >  static const struct regmap_config sun8i_mixer_regmap_config = {
> > -       .name           = "layers",
> > +       .name           = "display",
> >         .reg_bits       = 32,
> >         .val_bits       = 32,
> >         .reg_stride     = 4,
> > @@ -433,14 +440,6 @@ static const struct regmap_config sun8i_top_regmap_config = {
> >         .max_register   = 0x3c,
> >  };
> >
> > -static const struct regmap_config sun8i_disp_regmap_config = {
> > -       .name           = "display",
> > -       .reg_bits       = 32,
> > -       .val_bits       = 32,
> > -       .reg_stride     = 4,
> > -       .max_register   = 0x20000,
> > -};
> > -
> >  static int sun8i_mixer_of_get_id(struct device_node *node)
> >  {
> >         struct device_node *ep, *remote;
> > @@ -463,17 +462,14 @@ static int sun8i_mixer_of_get_id(struct device_node *node)
> >
> >  static void sun8i_mixer_init(struct sun8i_mixer *mixer)
> >  {
> > -       struct regmap *top_regs, *disp_regs;
> >         unsigned int base = sun8i_blender_base(mixer);
> > +       struct regmap *top_regs;
> >         int plane_cnt, i;
> >
> > -       if (mixer->cfg->de_type == SUN8I_MIXER_DE33) {
> > +       if (mixer->cfg->de_type == SUN8I_MIXER_DE33)
> >                 top_regs = mixer->top_regs;
> > -               disp_regs = mixer->disp_regs;
> > -       } else {
> > +       else
> >                 top_regs = mixer->engine.regs;
> > -               disp_regs = mixer->engine.regs;
> > -       }
> >
> >         /* Enable the mixer */
> >         regmap_write(top_regs, SUN8I_MIXER_GLOBAL_CTL,
> > @@ -483,25 +479,25 @@ static void sun8i_mixer_init(struct sun8i_mixer *mixer)
> >                 regmap_write(top_regs, SUN50I_MIXER_GLOBAL_CLK, 1);
> >
> >         /* Set background color to black */
> > -       regmap_write(disp_regs, SUN8I_MIXER_BLEND_BKCOLOR(base),
> > +       regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_BKCOLOR(base),
> >                      SUN8I_MIXER_BLEND_COLOR_BLACK);
> >
> >         /*
> >          * Set fill color of bottom plane to black. Generally not needed
> >          * except when VI plane is at bottom (zpos = 0) and enabled.
> >          */
> > -       regmap_write(disp_regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
> > +       regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
> >                      SUN8I_MIXER_BLEND_PIPE_CTL_FC_EN(0));
> > -       regmap_write(disp_regs, SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, 0),
> > +       regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, 0),
> >                      SUN8I_MIXER_BLEND_COLOR_BLACK);
> >
> >         plane_cnt = mixer->cfg->vi_num + mixer->cfg->ui_num;
> >         for (i = 0; i < plane_cnt; i++)
> > -               regmap_write(disp_regs,
> > +               regmap_write(mixer->engine.regs,
> >                              SUN8I_MIXER_BLEND_MODE(base, i),
> >                              SUN8I_MIXER_BLEND_MODE_DEF);
> >
> > -       regmap_update_bits(disp_regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
> > +       regmap_update_bits(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
> >                            SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, 0);
> >  }
> >
> > @@ -532,7 +528,6 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
> >         if (!mixer)
> >                 return -ENOMEM;
> >         dev_set_drvdata(dev, mixer);
> > -       mixer->engine.ops = &sun8i_engine_ops;
> >         mixer->engine.node = dev->of_node;
> >
> >         if (of_property_present(dev->of_node, "iommus")) {
> > @@ -562,6 +557,11 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
> >         if (!mixer->cfg)
> >                 return -EINVAL;
> >
> > +       if (mixer->cfg->de_type == SUN8I_MIXER_DE33)
> > +               mixer->engine.ops = &sun50i_engine_ops;
> 
> You're missing an IS_ENABLED() clause here if you wanted to make the DE 3.3
> planes driver optional. Though as I mentioned in the other patch, splittig
> the two modules might not work.

No, as I said in previous response, I'll merge it into sun8i-mixer.

> 
> > +       else
> > +               mixer->engine.ops = &sun8i_engine_ops;
> > +
> >         regs = devm_platform_ioremap_resource(pdev, 0);
> >         if (IS_ERR(regs))
> >                 return PTR_ERR(regs);
> > @@ -584,17 +584,6 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
> >                         dev_err(dev, "Couldn't create the top regmap\n");
> >                         return PTR_ERR(mixer->top_regs);
> >                 }
> > -
> > -               regs = devm_platform_ioremap_resource_byname(pdev, "display");
> > -               if (IS_ERR(regs))
> > -                       return PTR_ERR(regs);
> > -
> > -               mixer->disp_regs = devm_regmap_init_mmio(dev, regs,
> > -                                                        &sun8i_disp_regmap_config);
> > -               if (IS_ERR(mixer->disp_regs)) {
> > -                       dev_err(dev, "Couldn't create the disp regmap\n");
> > -                       return PTR_ERR(mixer->disp_regs);
> > -               }
> >         }
> >
> >         mixer->reset = devm_reset_control_get(dev, NULL);
> > @@ -634,6 +623,33 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
> >
> >         clk_prepare_enable(mixer->mod_clk);
> >
> > +       if (mixer->cfg->de_type == SUN8I_MIXER_DE33) {
> > +               struct platform_device *pdev;
> > +               struct device_node *np;
> > +               void *data;
> > +
> > +               np = of_parse_phandle(dev->of_node, "allwinner,planes", 0);
> > +               if (!np) {
> > +                       ret = -ENODEV;
> > +                       goto err_disable_mod_clk;
> > +               }
> > +
> > +               pdev = of_find_device_by_node(np);
> 
> You need to add a matching put_device() in the unbind function.
> 
> Side note:
> 
> This bind function is using a lot of devm_ functions. These have the wrong
> lifetime. I think it would be better if we could move resource acquisition
> into the probe function.

Good point. This can be clean up later.

> 
> 
> > +               of_node_put(np);
> > +               if (!pdev) {
> > +                       ret = -EPROBE_DEFER;
> > +                       goto err_disable_mod_clk;
> > +               }
> > +
> > +               data = platform_get_drvdata(pdev);
> > +               if (!data) {
> > +                       put_device(&pdev->dev);
> > +                       return -EPROBE_DEFER;
> 
> Should be a goto here?

Right.

Best regards,
Jernej

> 
> 
> ChenYu
> 
> > +               }
> > +
> > +               mixer->planes_dev = &pdev->dev;
> > +       }
> > +
> >         list_add_tail(&mixer->engine.list, &drv->engine_list);
> >
> >         /* Reset registers and disable unused sub-engines */
> > @@ -668,6 +684,8 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
> >
> >         return 0;
> >
> > +err_disable_mod_clk:
> > +       clk_disable_unprepare(mixer->mod_clk);
> >  err_disable_bus_clk:
> >         clk_disable_unprepare(mixer->bus_clk);
> >  err_assert_reset:
> > @@ -863,16 +881,8 @@ static const struct sun8i_mixer_cfg sun50i_h6_mixer0_cfg = {
> >  };
> >
> >  static const struct sun8i_mixer_cfg sun50i_h616_mixer0_cfg = {
> > -       .lay_cfg = {
> > -               .de_type        = SUN8I_MIXER_DE33,
> > -               .scaler_mask    = 0xf,
> > -               .scanline_yuv   = 4096,
> > -       },
> >         .de_type        = SUN8I_MIXER_DE33,
> >         .mod_rate       = 600000000,
> > -       .ui_num         = 3,
> > -       .vi_num         = 1,
> > -       .map            = {0, 6, 7, 8},
> >  };
> >
> >  static const struct of_device_id sun8i_mixer_of_table[] = {
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > index e2f83301aae8..7abc88c898d9 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > @@ -202,7 +202,6 @@ struct sun8i_mixer_cfg {
> >         int                     ui_num;
> >         unsigned int            de_type;
> >         unsigned long           mod_rate;
> > -       unsigned int            map[6];
> >  };
> >
> >  struct sun8i_mixer {
> > @@ -216,7 +215,7 @@ struct sun8i_mixer {
> >         struct clk                      *mod_clk;
> >
> >         struct regmap                   *top_regs;
> > -       struct regmap                   *disp_regs;
> > +       struct device                   *planes_dev;
> >  };
> >
> >  enum {
> > @@ -252,13 +251,6 @@ sun8i_blender_base(struct sun8i_mixer *mixer)
> >         return mixer->cfg->de_type == SUN8I_MIXER_DE3 ? DE3_BLD_BASE : DE2_BLD_BASE;
> >  }
> >
> > -static inline struct regmap *
> > -sun8i_blender_regmap(struct sun8i_mixer *mixer)
> > -{
> > -       return mixer->cfg->de_type == SUN8I_MIXER_DE33 ?
> > -               mixer->disp_regs : mixer->engine.regs;
> > -}
> > -
> >  static inline u32
> >  sun8i_channel_base(struct sun8i_layer *layer)
> >  {
> > --
> > 2.51.2
> >
> >
>