[PATCH v3 0/4] Add support for RZ/G2UL Display Unit

[PATCH v3 0/4] Add support for RZ/G2UL Display Unit

[Biju Das]

This patch series aims to add support for RZ/G2UL DU.

The LCD controller is composed of Frame Compression Processor (FCPVD),
Video Signal Processor (VSPD), and Display Unit (DU).

The output of LCDC is connected display parallel interface (DPI) and
supports a maximum resolution of WXGA along with 2 RPFs to support the
blending of two picture layers and raster operations (ROPs)

It is similar to LCDC IP on RZ/G2L SoCs, but does not have DSI interface.

v2->v3:
 * Split patch series based on subsystem from DU patch series [1].
 * Replaced ports->port property for RZ/G2UL as it supports only DPI
   and retained ports property for RZ/{G2L,V2L} as it supports both DSI
   and DPI output interface.
 * Added missing blank line before example.
 * Dropped tags from Conor and Geert as there are new changes in bindings
 * Avoided the line break in rzg2l_du_start_stop() for rstate.
 * Replaced port->du_output in  struct rzg2l_du_output_routing and
   dropped using the port number to indicate the output type in
   rzg2l_du_encoders_init().
 * Updated rzg2l_du_r9a07g043u_info and rzg2l_du_r9a07g044_info.

 [1] https://lore.kernel.org/all/20240709135152.185042-1-biju.das.jz@bp.renesas.com/
v1->v2:
 * Updated cover letter header "DU IP->Display Unit".
 * Updated commit description related to non ABI breakage for patch#3.
 * Added Ack from Conor for binding patches.

Biju Das (4):
  dt-bindings: display: renesas,rzg2l-du: Document RZ/G2UL DU bindings
  drm: renesas: rz-du: Add RZ/G2UL DU Support
  arm64: dts: renesas: r9a07g043u: Add DU node
  arm64: dts: renesas: r9a07g043u11-smarc: Enable DU

 .../bindings/display/renesas,rzg2l-du.yaml    |  35 +++++-
 arch/arm64/boot/dts/renesas/r9a07g043u.dtsi   |  19 +++
 .../boot/dts/renesas/r9a07g043u11-smarc.dts   | 109 ++++++++++++++++++
 drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c |   8 +-
 drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c  |  18 ++-
 drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h  |   5 +-
 drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c  |   4 +-
 7 files changed, 188 insertions(+), 10 deletions(-)

-- 
2.43.0

[PATCH v3 1/4] dt-bindings: display: renesas,rzg2l-du: Document RZ/G2UL DU bindings

[Biju Das]

Document DU found in RZ/G2UL SoC. The DU block is identical to RZ/G2L
SoC, but has only DPI interface.

While at it, add missing required property port@1 for RZ/G2L and RZ/V2L
SoCs. Currently there is no user for the DPI interface and hence there
won't be any ABI breakage for adding port@1 as required property for
RZ/G2L and RZ/V2L SoCs.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * Replaced ports->port property for RZ/G2UL as it supports only DPI.
   and retained ports property for RZ/{G2L,V2L} as it supports both DSI
   and DPI output interface.
 * Added missing blank line before example.
 * Dropped tags from Conor and Geert as there are new changes.
v1->v2:
 * Updated commit description related to non ABI breakage.
 * Added Ack from Conor.
---
 .../bindings/display/renesas,rzg2l-du.yaml    | 35 +++++++++++++++++--
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
index 08e5b9478051..ca01bf26c4c0 100644
--- a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
+++ b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
@@ -18,6 +18,7 @@ properties:
   compatible:
     oneOf:
       - enum:
+          - renesas,r9a07g043u-du # RZ/G2UL
           - renesas,r9a07g044-du # RZ/G2{L,LC}
       - items:
           - enum:
@@ -60,8 +61,9 @@ properties:
         $ref: /schemas/graph.yaml#/properties/port
         unevaluatedProperties: false
 
-    required:
-      - port@0
+  port:
+    $ref: /schemas/graph.yaml#/properties/port
+    description: Connection to the DU output video port.
 
     unevaluatedProperties: false
 
@@ -83,11 +85,38 @@ required:
   - clock-names
   - resets
   - power-domains
-  - ports
   - renesas,vsps
 
 additionalProperties: false
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: renesas,r9a07g043u-du
+    then:
+      properties:
+        port:
+          description: DPI
+
+      required:
+        - port
+    else:
+      properties:
+        ports:
+          properties:
+            port@0:
+              description: DSI
+            port@1:
+              description: DPI
+
+          required:
+            - port@0
+            - port@1
+      required:
+        - ports
+
 examples:
   # RZ/G2L DU
   - |
-- 
2.43.0

[PATCH v3 1/4] dt-bindings: display: renesas, rzg2l-du: Document RZ/G2UL DU bindings

[Biju Das]

Document DU found in RZ/G2UL SoC. The DU block is identical to RZ/G2L
SoC, but has only DPI interface.

While at it, add missing required property port@1 for RZ/G2L and RZ/V2L
SoCs. Currently there is no user for the DPI interface and hence there
won't be any ABI breakage for adding port@1 as required property for
RZ/G2L and RZ/V2L SoCs.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * Replaced ports->port property for RZ/G2UL as it supports only DPI.
   and retained ports property for RZ/{G2L,V2L} as it supports both DSI
   and DPI output interface.
 * Added missing blank line before example.
 * Dropped tags from Conor and Geert as there are new changes.
v1->v2:
 * Updated commit description related to non ABI breakage.
 * Added Ack from Conor.
---
 .../bindings/display/renesas,rzg2l-du.yaml    | 35 +++++++++++++++++--
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
index 08e5b9478051..ca01bf26c4c0 100644
--- a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
+++ b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
@@ -18,6 +18,7 @@ properties:
   compatible:
     oneOf:
       - enum:
+          - renesas,r9a07g043u-du # RZ/G2UL
           - renesas,r9a07g044-du # RZ/G2{L,LC}
       - items:
           - enum:
@@ -60,8 +61,9 @@ properties:
         $ref: /schemas/graph.yaml#/properties/port
         unevaluatedProperties: false
 
-    required:
-      - port@0
+  port:
+    $ref: /schemas/graph.yaml#/properties/port
+    description: Connection to the DU output video port.
 
     unevaluatedProperties: false
 
@@ -83,11 +85,38 @@ required:
   - clock-names
   - resets
   - power-domains
-  - ports
   - renesas,vsps
 
 additionalProperties: false
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: renesas,r9a07g043u-du
+    then:
+      properties:
+        port:
+          description: DPI
+
+      required:
+        - port
+    else:
+      properties:
+        ports:
+          properties:
+            port@0:
+              description: DSI
+            port@1:
+              description: DPI
+
+          required:
+            - port@0
+            - port@1
+      required:
+        - ports
+
 examples:
   # RZ/G2L DU
   - |
-- 
2.43.0

[PATCH v3 2/4] drm: renesas: rz-du: Add RZ/G2UL DU Support

[Biju Das]

The LCD controller is composed of Frame Compression Processor (FCPVD),
Video Signal Processor (VSPD), and Display Unit (DU).

It has DPI interface and supports a maximum resolution of WXGA along
with 2 RPFs to support the blending of two picture layers and raster
operations (ROPs).

The DU module is connected to VSPD. Add RZ/G2UL DU support.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * Avoided the line break in rzg2l_du_start_stop() for rstate.
 * Replaced port->du_output in  struct rzg2l_du_output_routing and
   dropped using the port number to indicate the output type in
   rzg2l_du_encoders_init().
 * Updated rzg2l_du_r9a07g043u_info and rzg2l_du_r9a07g044_info
v1->v2:
 * No change.
---
 drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c |  8 +++++++-
 drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c  | 18 ++++++++++++++++--
 drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h  |  5 +++--
 drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c  |  4 ++--
 4 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
index 6e7aac6219be..fd7675c7f181 100644
--- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
+++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
@@ -28,6 +28,7 @@
 #include "rzg2l_du_vsp.h"
 
 #define DU_MCR0			0x00
+#define DU_MCR0_DPI_OE		BIT(0)
 #define DU_MCR0_DI_EN		BIT(8)
 
 #define DU_DITR0		0x10
@@ -216,9 +217,14 @@ static void rzg2l_du_crtc_put(struct rzg2l_du_crtc *rcrtc)
 
 static void rzg2l_du_start_stop(struct rzg2l_du_crtc *rcrtc, bool start)
 {
+	struct rzg2l_du_crtc_state *rstate = to_rzg2l_crtc_state(rcrtc->crtc.state);
 	struct rzg2l_du_device *rcdu = rcrtc->dev;
+	u32 val = DU_MCR0_DI_EN;
 
-	writel(start ? DU_MCR0_DI_EN : 0, rcdu->mmio + DU_MCR0);
+	if (rstate->outputs == BIT(RZG2L_DU_OUTPUT_DPAD0))
+		val |= DU_MCR0_DPI_OE;
+
+	writel(start ? val : 0, rcdu->mmio + DU_MCR0);
 }
 
 static void rzg2l_du_crtc_start(struct rzg2l_du_crtc *rcrtc)
diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
index e5eca8691a33..69b8e216ee1a 100644
--- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
+++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
@@ -25,21 +25,35 @@
  * Device Information
  */
 
+static const struct rzg2l_du_device_info rzg2l_du_r9a07g043u_info = {
+	.channels_mask = BIT(0),
+	.routes = {
+		[RZG2L_DU_OUTPUT_DSI0] = {
+			.du_output = RZG2L_DU_OUTPUT_INVALID,
+		},
+		[RZG2L_DU_OUTPUT_DPAD0] = {
+			.possible_outputs = BIT(0),
+			.du_output = RZG2L_DU_OUTPUT_DPAD0,
+		},
+	},
+};
+
 static const struct rzg2l_du_device_info rzg2l_du_r9a07g044_info = {
 	.channels_mask = BIT(0),
 	.routes = {
 		[RZG2L_DU_OUTPUT_DSI0] = {
 			.possible_outputs = BIT(0),
-			.port = 0,
+			.du_output = RZG2L_DU_OUTPUT_DSI0,
 		},
 		[RZG2L_DU_OUTPUT_DPAD0] = {
 			.possible_outputs = BIT(0),
-			.port = 1,
+			.du_output = RZG2L_DU_OUTPUT_DPAD0,
 		}
 	}
 };
 
 static const struct of_device_id rzg2l_du_of_table[] = {
+	{ .compatible = "renesas,r9a07g043u-du", .data = &rzg2l_du_r9a07g043u_info },
 	{ .compatible = "renesas,r9a07g044-du", .data = &rzg2l_du_r9a07g044_info },
 	{ /* sentinel */ }
 };
diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h
index 58806c2a8f2b..ab82b5c86d6e 100644
--- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h
+++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h
@@ -29,7 +29,7 @@ enum rzg2l_du_output {
 /*
  * struct rzg2l_du_output_routing - Output routing specification
  * @possible_outputs: bitmask of possible outputs
- * @port: device tree port number corresponding to this output route
+ * @du_output: DU output
  *
  * The DU has 2 possible outputs (DPAD0, DSI0). Output routing data
  * specify the valid SoC outputs, which CRTC can drive the output, and the type
@@ -37,7 +37,7 @@ enum rzg2l_du_output {
  */
 struct rzg2l_du_output_routing {
 	unsigned int possible_outputs;
-	unsigned int port;
+	unsigned int du_output;
 };
 
 /*
@@ -53,6 +53,7 @@ struct rzg2l_du_device_info {
 #define RZG2L_DU_MAX_CRTCS		1
 #define RZG2L_DU_MAX_VSPS		1
 #define RZG2L_DU_MAX_DSI		1
+#define RZG2L_DU_OUTPUT_INVALID		-1
 
 struct rzg2l_du_device {
 	struct device *dev;
diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
index 07b312b6f81e..361350f2999e 100644
--- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
+++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
@@ -183,8 +183,8 @@ static int rzg2l_du_encoders_init(struct rzg2l_du_device *rcdu)
 
 		/* Find the output route corresponding to the port number. */
 		for (i = 0; i < RZG2L_DU_OUTPUT_MAX; ++i) {
-			if (rcdu->info->routes[i].port == ep.port) {
-				output = i;
+			if (i == rcdu->info->routes[i].du_output) {
+				output = rcdu->info->routes[i].du_output;
 				break;
 			}
 		}
-- 
2.43.0

[PATCH v3 3/4] arm64: dts: renesas: r9a07g043u: Add DU node

[Biju Das]

Add DU node to RZ/G2UL SoC DTSI.

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * Dropped ports->port as it supports only DPI
 * Added Rb tag from Laurent.
v1->v2:
 * No change.
---
 arch/arm64/boot/dts/renesas/r9a07g043u.dtsi | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi b/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
index d88bf23b0782..ba88c91c66c8 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
@@ -153,6 +153,25 @@ fcpvd: fcp@10880000 {
 		resets = <&cpg R9A07G043_LCDC_RESET_N>;
 	};
 
+	du: display@10890000 {
+		compatible = "renesas,r9a07g043u-du";
+		reg = <0 0x10890000 0 0x10000>;
+		interrupts = <SOC_PERIPHERAL_IRQ(152) IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cpg CPG_MOD R9A07G043_LCDC_CLK_A>,
+			 <&cpg CPG_MOD R9A07G043_LCDC_CLK_P>,
+			 <&cpg CPG_MOD R9A07G043_LCDC_CLK_D>;
+		clock-names = "aclk", "pclk", "vclk";
+		power-domains = <&cpg>;
+		resets = <&cpg R9A07G043_LCDC_RESET_N>;
+		renesas,vsps = <&vspd 0>;
+		status = "disabled";
+
+		port {
+			du_out_rgb: endpoint {
+			};
+		};
+	};
+
 	irqc: interrupt-controller@110a0000 {
 		compatible = "renesas,r9a07g043u-irqc",
 			     "renesas,rzg2l-irqc";
-- 
2.43.0

[PATCH v3 4/4] arm64: dts: renesas: r9a07g043u11-smarc: Enable DU

[Biju Das]

Enable DU and link with the HDMI add-on board connected with
the parallel connector on RZ/G2UL SMARC EVK.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * Replaced ports->port in du node as it support only DPI output.
v1->v2:
 * No change.
---
 .../boot/dts/renesas/r9a07g043u11-smarc.dts   | 109 ++++++++++++++++++
 1 file changed, 109 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r9a07g043u11-smarc.dts b/arch/arm64/boot/dts/renesas/r9a07g043u11-smarc.dts
index 8e0107df2d46..418902e1370e 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g043u11-smarc.dts
+++ b/arch/arm64/boot/dts/renesas/r9a07g043u11-smarc.dts
@@ -35,4 +35,113 @@
 / {
 	model = "Renesas SMARC EVK based on r9a07g043u11";
 	compatible = "renesas,smarc-evk", "renesas,r9a07g043u11", "renesas,r9a07g043";
+
+	hdmi-out {
+		compatible = "hdmi-connector";
+		type = "d";
+
+		port {
+			hdmi_con_out: endpoint {
+				remote-endpoint = <&adv7513_out>;
+			};
+		};
+	};
+};
+
+&du {
+	pinctrl-0 = <&du_pins>;
+	pinctrl-names = "default";
+
+	status = "okay";
+
+	port {
+		du_out_rgb: endpoint {
+			remote-endpoint = <&adv7513_in>;
+		};
+	};
+};
+
+&i2c1 {
+	adv7513: adv7513@39 {
+		compatible = "adi,adv7513";
+		reg = <0x39>;
+
+		adi,input-depth = <8>;
+		adi,input-colorspace = "rgb";
+		adi,input-clock = "1x";
+
+		avdd-supply = <&reg_1p8v>;
+		dvdd-supply = <&reg_1p8v>;
+		pvdd-supply = <&reg_1p8v>;
+		dvdd-3v-supply = <&reg_3p3v>;
+		bgvdd-supply = <&reg_1p8v>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+
+				adv7513_in: endpoint {
+					remote-endpoint = <&du_out_rgb>;
+				};
+			};
+
+			port@1 {
+				reg = <1>;
+
+				adv7513_out: endpoint {
+					remote-endpoint = <&hdmi_con_out>;
+				};
+			};
+		};
+	};
+};
+
+&pinctrl {
+	du_pins: du {
+		data {
+			pinmux = <RZG2L_PORT_PINMUX(11, 2, 6)>,
+				 <RZG2L_PORT_PINMUX(13, 1, 6)>,
+				 <RZG2L_PORT_PINMUX(13, 0, 6)>,
+				 <RZG2L_PORT_PINMUX(13, 4, 6)>,
+				 <RZG2L_PORT_PINMUX(13, 3, 6)>,
+				 <RZG2L_PORT_PINMUX(12, 1, 6)>,
+				 <RZG2L_PORT_PINMUX(13, 2, 6)>,
+				 <RZG2L_PORT_PINMUX(14, 0, 6)>,
+				 <RZG2L_PORT_PINMUX(14, 2, 6)>,
+				 <RZG2L_PORT_PINMUX(14, 1, 6)>,
+				 <RZG2L_PORT_PINMUX(16, 0, 6)>,
+				 <RZG2L_PORT_PINMUX(15, 0, 6)>,
+				 <RZG2L_PORT_PINMUX(16, 1, 6)>,
+				 <RZG2L_PORT_PINMUX(15, 1, 6)>,
+				 <RZG2L_PORT_PINMUX(15, 3, 6)>,
+				 <RZG2L_PORT_PINMUX(18, 0, 6)>,
+				 <RZG2L_PORT_PINMUX(15, 2, 6)>,
+				 <RZG2L_PORT_PINMUX(17, 0, 6)>,
+				 <RZG2L_PORT_PINMUX(17, 2, 6)>,
+				 <RZG2L_PORT_PINMUX(17, 1, 6)>,
+				 <RZG2L_PORT_PINMUX(18, 1, 6)>,
+				 <RZG2L_PORT_PINMUX(18, 2, 6)>,
+				 <RZG2L_PORT_PINMUX(17, 3, 6)>,
+				 <RZG2L_PORT_PINMUX(18, 3, 6)>;
+			drive-strength = <2>;
+		};
+
+		sync {
+			pinmux = <RZG2L_PORT_PINMUX(11, 0, 6)>, /* HSYNC */
+				 <RZG2L_PORT_PINMUX(12, 0, 6)>; /* VSYNC */
+			drive-strength = <2>;
+		};
+
+		de {
+			pinmux = <RZG2L_PORT_PINMUX(11, 1, 6)>; /* DE */
+			drive-strength = <2>;
+		};
+
+		clk {
+			pinmux = <RZG2L_PORT_PINMUX(11, 3, 6)>; /* CLK */
+		};
+	};
 };
-- 
2.43.0

Re: [PATCH v3 1/4] dt-bindings: display: renesas,rzg2l-du: Document RZ/G2UL DU bindings

[Rob Herring]

On Mon, Aug 05, 2024 at 04:52:35PM +0100, Biju Das wrote:
> Document DU found in RZ/G2UL SoC. The DU block is identical to RZ/G2L
> SoC, but has only DPI interface.
> 
> While at it, add missing required property port@1 for RZ/G2L and RZ/V2L
> SoCs. Currently there is no user for the DPI interface and hence there
> won't be any ABI breakage for adding port@1 as required property for
> RZ/G2L and RZ/V2L SoCs.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2->v3:
>  * Replaced ports->port property for RZ/G2UL as it supports only DPI.
>    and retained ports property for RZ/{G2L,V2L} as it supports both DSI
>    and DPI output interface.

Why? Having port and ports is just a needless complication.

>  * Added missing blank line before example.
>  * Dropped tags from Conor and Geert as there are new changes.
> v1->v2:
>  * Updated commit description related to non ABI breakage.
>  * Added Ack from Conor.
> ---
>  .../bindings/display/renesas,rzg2l-du.yaml    | 35 +++++++++++++++++--
>  1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> index 08e5b9478051..ca01bf26c4c0 100644
> --- a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> +++ b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> @@ -18,6 +18,7 @@ properties:
>    compatible:
>      oneOf:
>        - enum:
> +          - renesas,r9a07g043u-du # RZ/G2UL
>            - renesas,r9a07g044-du # RZ/G2{L,LC}
>        - items:
>            - enum:
> @@ -60,8 +61,9 @@ properties:
>          $ref: /schemas/graph.yaml#/properties/port
>          unevaluatedProperties: false
>  
> -    required:
> -      - port@0
> +  port:
> +    $ref: /schemas/graph.yaml#/properties/port
> +    description: Connection to the DU output video port.
>  
>      unevaluatedProperties: false
>  
> @@ -83,11 +85,38 @@ required:
>    - clock-names
>    - resets
>    - power-domains
> -  - ports
>    - renesas,vsps
>  
>  additionalProperties: false
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: renesas,r9a07g043u-du
> +    then:
> +      properties:
> +        port:
> +          description: DPI

This is equivalent to 'port@0'. IMO, this case should have a 'port@1' 
node so that DPI interface is *always* the same port.

> +
> +      required:
> +        - port
> +    else:
> +      properties:
> +        ports:
> +          properties:
> +            port@0:
> +              description: DSI
> +            port@1:
> +              description: DPI
> +
> +          required:
> +            - port@0
> +            - port@1
> +      required:
> +        - ports
> +
>  examples:
>    # RZ/G2L DU
>    - |
> -- 
> 2.43.0
> 

Re: [PATCH v3 1/4] dt-bindings: display: renesas,rzg2l-du: Document RZ/G2UL DU bindings

[Laurent Pinchart]

Hi Rob,

On Tue, Aug 13, 2024 at 10:32:20AM -0600, Rob Herring wrote:
> On Mon, Aug 05, 2024 at 04:52:35PM +0100, Biju Das wrote:
> > Document DU found in RZ/G2UL SoC. The DU block is identical to RZ/G2L
> > SoC, but has only DPI interface.
> > 
> > While at it, add missing required property port@1 for RZ/G2L and RZ/V2L
> > SoCs. Currently there is no user for the DPI interface and hence there
> > won't be any ABI breakage for adding port@1 as required property for
> > RZ/G2L and RZ/V2L SoCs.
> > 
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v2->v3:
> >  * Replaced ports->port property for RZ/G2UL as it supports only DPI.
> >    and retained ports property for RZ/{G2L,V2L} as it supports both DSI
> >    and DPI output interface.
> 
> Why? Having port and ports is just a needless complication.

I agree that making the ports node mandatory, even when the device has a
single port, will simplify the bindings. In hindsight we should never
have made ports optional, but that can't be changed.

> >  * Added missing blank line before example.
> >  * Dropped tags from Conor and Geert as there are new changes.
> > v1->v2:
> >  * Updated commit description related to non ABI breakage.
> >  * Added Ack from Conor.
> > ---
> >  .../bindings/display/renesas,rzg2l-du.yaml    | 35 +++++++++++++++++--
> >  1 file changed, 32 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > index 08e5b9478051..ca01bf26c4c0 100644
> > --- a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > +++ b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > @@ -18,6 +18,7 @@ properties:
> >    compatible:
> >      oneOf:
> >        - enum:
> > +          - renesas,r9a07g043u-du # RZ/G2UL
> >            - renesas,r9a07g044-du # RZ/G2{L,LC}
> >        - items:
> >            - enum:
> > @@ -60,8 +61,9 @@ properties:
> >          $ref: /schemas/graph.yaml#/properties/port
> >          unevaluatedProperties: false
> >  
> > -    required:
> > -      - port@0
> > +  port:
> > +    $ref: /schemas/graph.yaml#/properties/port
> > +    description: Connection to the DU output video port.
> >  
> >      unevaluatedProperties: false
> >  
> > @@ -83,11 +85,38 @@ required:
> >    - clock-names
> >    - resets
> >    - power-domains
> > -  - ports
> >    - renesas,vsps
> >  
> >  additionalProperties: false
> >  
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: renesas,r9a07g043u-du
> > +    then:
> > +      properties:
> > +        port:
> > +          description: DPI
> 
> This is equivalent to 'port@0'. IMO, this case should have a 'port@1' 
> node so that DPI interface is *always* the same port.

That's what Biju did in the previous version, and I recommended to
number the ports based on hardware indices, not types. Mapping port
numbers to the hardware documentation makes it more consistent for DT
writers, makes the logic simpler to understand (in my opinion, based on
my experience with the R-Car DU) on the driver side, but most
importantly, type-based numbering wouldn't scale as SoCs could have
multiple ports of the same type (we've seen that happening with R-Car).

> > +
> > +      required:
> > +        - port
> > +    else:
> > +      properties:
> > +        ports:
> > +          properties:
> > +            port@0:
> > +              description: DSI
> > +            port@1:
> > +              description: DPI
> > +
> > +          required:
> > +            - port@0
> > +            - port@1
> > +      required:
> > +        - ports
> > +
> >  examples:
> >    # RZ/G2L DU
> >    - |

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 2/4] drm: renesas: rz-du: Add RZ/G2UL DU Support

[Laurent Pinchart]

Hi Biju,

Thank you for the patch.

On Mon, Aug 05, 2024 at 04:52:36PM +0100, Biju Das wrote:
> The LCD controller is composed of Frame Compression Processor (FCPVD),
> Video Signal Processor (VSPD), and Display Unit (DU).
> 
> It has DPI interface and supports a maximum resolution of WXGA along
> with 2 RPFs to support the blending of two picture layers and raster
> operations (ROPs).
> 
> The DU module is connected to VSPD. Add RZ/G2UL DU support.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2->v3:
>  * Avoided the line break in rzg2l_du_start_stop() for rstate.
>  * Replaced port->du_output in  struct rzg2l_du_output_routing and
>    dropped using the port number to indicate the output type in
>    rzg2l_du_encoders_init().
>  * Updated rzg2l_du_r9a07g043u_info and rzg2l_du_r9a07g044_info
> v1->v2:
>  * No change.
> ---
>  drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c |  8 +++++++-
>  drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c  | 18 ++++++++++++++++--
>  drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h  |  5 +++--
>  drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c  |  4 ++--
>  4 files changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> index 6e7aac6219be..fd7675c7f181 100644
> --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> @@ -28,6 +28,7 @@
>  #include "rzg2l_du_vsp.h"
>  
>  #define DU_MCR0			0x00
> +#define DU_MCR0_DPI_OE		BIT(0)
>  #define DU_MCR0_DI_EN		BIT(8)
>  
>  #define DU_DITR0		0x10
> @@ -216,9 +217,14 @@ static void rzg2l_du_crtc_put(struct rzg2l_du_crtc *rcrtc)
>  
>  static void rzg2l_du_start_stop(struct rzg2l_du_crtc *rcrtc, bool start)
>  {
> +	struct rzg2l_du_crtc_state *rstate = to_rzg2l_crtc_state(rcrtc->crtc.state);
>  	struct rzg2l_du_device *rcdu = rcrtc->dev;
> +	u32 val = DU_MCR0_DI_EN;
>  
> -	writel(start ? DU_MCR0_DI_EN : 0, rcdu->mmio + DU_MCR0);
> +	if (rstate->outputs == BIT(RZG2L_DU_OUTPUT_DPAD0))

Is output supposed to contain a single bit, or can it contain multiple
bits ? In the first case I would rename it to output, in the second case
you should probably use '&' instead of '=='.

> +		val |= DU_MCR0_DPI_OE;
> +
> +	writel(start ? val : 0, rcdu->mmio + DU_MCR0);
>  }
>  
>  static void rzg2l_du_crtc_start(struct rzg2l_du_crtc *rcrtc)
> diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> index e5eca8691a33..69b8e216ee1a 100644
> --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> @@ -25,21 +25,35 @@
>   * Device Information
>   */
>  
> +static const struct rzg2l_du_device_info rzg2l_du_r9a07g043u_info = {
> +	.channels_mask = BIT(0),
> +	.routes = {
> +		[RZG2L_DU_OUTPUT_DSI0] = {
> +			.du_output = RZG2L_DU_OUTPUT_INVALID,
> +		},

You can drop this entry, as well as the RZG2L_DU_OUTPUT_INVALID macro.
See below.

> +		[RZG2L_DU_OUTPUT_DPAD0] = {
> +			.possible_outputs = BIT(0),
> +			.du_output = RZG2L_DU_OUTPUT_DPAD0,
> +		},
> +	},
> +};
> +
>  static const struct rzg2l_du_device_info rzg2l_du_r9a07g044_info = {
>  	.channels_mask = BIT(0),
>  	.routes = {
>  		[RZG2L_DU_OUTPUT_DSI0] = {
>  			.possible_outputs = BIT(0),
> -			.port = 0,
> +			.du_output = RZG2L_DU_OUTPUT_DSI0,
>  		},
>  		[RZG2L_DU_OUTPUT_DPAD0] = {
>  			.possible_outputs = BIT(0),
> -			.port = 1,
> +			.du_output = RZG2L_DU_OUTPUT_DPAD0,
>  		}
>  	}
>  };
>  
>  static const struct of_device_id rzg2l_du_of_table[] = {
> +	{ .compatible = "renesas,r9a07g043u-du", .data = &rzg2l_du_r9a07g043u_info },
>  	{ .compatible = "renesas,r9a07g044-du", .data = &rzg2l_du_r9a07g044_info },
>  	{ /* sentinel */ }
>  };
> diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h
> index 58806c2a8f2b..ab82b5c86d6e 100644
> --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h
> +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h
> @@ -29,7 +29,7 @@ enum rzg2l_du_output {
>  /*
>   * struct rzg2l_du_output_routing - Output routing specification
>   * @possible_outputs: bitmask of possible outputs
> - * @port: device tree port number corresponding to this output route
> + * @du_output: DU output
>   *
>   * The DU has 2 possible outputs (DPAD0, DSI0). Output routing data
>   * specify the valid SoC outputs, which CRTC can drive the output, and the type
> @@ -37,7 +37,7 @@ enum rzg2l_du_output {
>   */
>  struct rzg2l_du_output_routing {
>  	unsigned int possible_outputs;
> -	unsigned int port;
> +	unsigned int du_output;
>  };
>  
>  /*
> @@ -53,6 +53,7 @@ struct rzg2l_du_device_info {
>  #define RZG2L_DU_MAX_CRTCS		1
>  #define RZG2L_DU_MAX_VSPS		1
>  #define RZG2L_DU_MAX_DSI		1
> +#define RZG2L_DU_OUTPUT_INVALID		-1
>  
>  struct rzg2l_du_device {
>  	struct device *dev;
> diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> index 07b312b6f81e..361350f2999e 100644
> --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> @@ -183,8 +183,8 @@ static int rzg2l_du_encoders_init(struct rzg2l_du_device *rcdu)
>  
>  		/* Find the output route corresponding to the port number. */
>  		for (i = 0; i < RZG2L_DU_OUTPUT_MAX; ++i) {
> -			if (rcdu->info->routes[i].port == ep.port) {
> -				output = i;
> +			if (i == rcdu->info->routes[i].du_output) {

If I understand the code correctly, this will always be true except for
the routes marked with RZG2L_DU_OUTPUT_INVALID, so you will match the
first valid route, regardless of the value of ep.port. I don't think
that's correct.

I would keep the port field in the rzg2l_du_output_routing, drop the
newly added du_output field, and use the following logic:

		for (i = 0; i < RZG2L_DU_OUTPUT_MAX; ++i) {
			if (rcdu->info->routes[i].possible_outputs &&
			    rcdu->info->routes[i].port == ep.port) {
				output = i;
				break;
			}
		}

Testing possible_outputs skips the routes that don't exist for the
device, and the ep.port comparison picks the route corresponding to the
port.

> +				output = rcdu->info->routes[i].du_output;
>  				break;
>  			}
>  		}

-- 
Regards,

Laurent Pinchart

RE: [PATCH v3 1/4] dt-bindings: display: renesas,rzg2l-du: Document RZ/G2UL DU bindings

[Biju Das]

Hi Laurent and Rob,

Thanks for the feedback

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Tuesday, August 13, 2024 8:39 PM
> Subject: Re: [PATCH v3 1/4] dt-bindings: display: renesas,rzg2l-du: Document RZ/G2UL DU bindings
> 
> Hi Rob,
> 
> On Tue, Aug 13, 2024 at 10:32:20AM -0600, Rob Herring wrote:
> > On Mon, Aug 05, 2024 at 04:52:35PM +0100, Biju Das wrote:
> > > Document DU found in RZ/G2UL SoC. The DU block is identical to
> > > RZ/G2L SoC, but has only DPI interface.
> > >
> > > While at it, add missing required property port@1 for RZ/G2L and
> > > RZ/V2L SoCs. Currently there is no user for the DPI interface and
> > > hence there won't be any ABI breakage for adding port@1 as required
> > > property for RZ/G2L and RZ/V2L SoCs.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > > v2->v3:
> > >  * Replaced ports->port property for RZ/G2UL as it supports only DPI.
> > >    and retained ports property for RZ/{G2L,V2L} as it supports both DSI
> > >    and DPI output interface.
> >
> > Why? Having port and ports is just a needless complication.
> 
> I agree that making the ports node mandatory, even when the device has a single port, will simplify
> the bindings. In hindsight we should never have made ports optional, but that can't be changed.
> 
> > >  * Added missing blank line before example.
> > >  * Dropped tags from Conor and Geert as there are new changes.
> > > v1->v2:
> > >  * Updated commit description related to non ABI breakage.
> > >  * Added Ack from Conor.
> > > ---
> > >  .../bindings/display/renesas,rzg2l-du.yaml    | 35 +++++++++++++++++--
> > >  1 file changed, 32 insertions(+), 3 deletions(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > > b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > > index 08e5b9478051..ca01bf26c4c0 100644
> > > ---
> > > a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > > +++ b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yam
> > > +++ l
> > > @@ -18,6 +18,7 @@ properties:
> > >    compatible:
> > >      oneOf:
> > >        - enum:
> > > +          - renesas,r9a07g043u-du # RZ/G2UL
> > >            - renesas,r9a07g044-du # RZ/G2{L,LC}
> > >        - items:
> > >            - enum:
> > > @@ -60,8 +61,9 @@ properties:
> > >          $ref: /schemas/graph.yaml#/properties/port
> > >          unevaluatedProperties: false
> > >
> > > -    required:
> > > -      - port@0
> > > +  port:
> > > +    $ref: /schemas/graph.yaml#/properties/port
> > > +    description: Connection to the DU output video port.
> > >
> > >      unevaluatedProperties: false
> > >
> > > @@ -83,11 +85,38 @@ required:
> > >    - clock-names
> > >    - resets
> > >    - power-domains
> > > -  - ports
> > >    - renesas,vsps
> > >
> > >  additionalProperties: false
> > >
> > > +allOf:
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: renesas,r9a07g043u-du
> > > +    then:
> > > +      properties:
> > > +        port:
> > > +          description: DPI
> >
> > This is equivalent to 'port@0'. IMO, this case should have a 'port@1'
> > node so that DPI interface is *always* the same port.
> 
> That's what Biju did in the previous version, and I recommended to number the ports based on hardware
> indices, not types. Mapping port numbers to the hardware documentation makes it more consistent for DT
> writers, makes the logic simpler to understand (in my opinion, based on my experience with the R-Car
> DU) on the driver side, but most importantly, type-based numbering wouldn't scale as SoCs could have
> multiple ports of the same type (we've seen that happening with R-Car).

OK, I will send bindings based on hardware indices.

Cheers,
Biju

> 
> > > +
> > > +      required:
> > > +        - port
> > > +    else:
> > > +      properties:
> > > +        ports:
> > > +          properties:
> > > +            port@0:
> > > +              description: DSI
> > > +            port@1:
> > > +              description: DPI
> > > +
> > > +          required:
> > > +            - port@0
> > > +            - port@1
> > > +      required:
> > > +        - ports
> > > +
> > >  examples:
> > >    # RZ/G2L DU
> > >    - |
> 
> --
> Regards,
> 
> Laurent Pinchart

RE: [PATCH v3 2/4] drm: renesas: rz-du: Add RZ/G2UL DU Support

[Biju Das]

Hi Laurent,

Thanks for the feedback.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Tuesday, August 13, 2024 8:49 PM
> Subject: Re: [PATCH v3 2/4] drm: renesas: rz-du: Add RZ/G2UL DU Support
> 
> Hi Biju,
> 
> Thank you for the patch.
> 
> On Mon, Aug 05, 2024 at 04:52:36PM +0100, Biju Das wrote:
> > The LCD controller is composed of Frame Compression Processor (FCPVD),
> > Video Signal Processor (VSPD), and Display Unit (DU).
> >
> > It has DPI interface and supports a maximum resolution of WXGA along
> > with 2 RPFs to support the blending of two picture layers and raster
> > operations (ROPs).
> >
> > The DU module is connected to VSPD. Add RZ/G2UL DU support.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v2->v3:
> >  * Avoided the line break in rzg2l_du_start_stop() for rstate.
> >  * Replaced port->du_output in  struct rzg2l_du_output_routing and
> >    dropped using the port number to indicate the output type in
> >    rzg2l_du_encoders_init().
> >  * Updated rzg2l_du_r9a07g043u_info and rzg2l_du_r9a07g044_info
> > v1->v2:
> >  * No change.
> > ---
> >  drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c |  8 +++++++-
> > drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c  | 18 ++++++++++++++++--
> > drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h  |  5 +++--
> > drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c  |  4 ++--
> >  4 files changed, 28 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> > b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> > index 6e7aac6219be..fd7675c7f181 100644
> > --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> > @@ -28,6 +28,7 @@
> >  #include "rzg2l_du_vsp.h"
> >
> >  #define DU_MCR0			0x00
> > +#define DU_MCR0_DPI_OE		BIT(0)
> >  #define DU_MCR0_DI_EN		BIT(8)
> >
> >  #define DU_DITR0		0x10
> > @@ -216,9 +217,14 @@ static void rzg2l_du_crtc_put(struct
> > rzg2l_du_crtc *rcrtc)
> >
> >  static void rzg2l_du_start_stop(struct rzg2l_du_crtc *rcrtc, bool
> > start)  {
> > +	struct rzg2l_du_crtc_state *rstate =
> > +to_rzg2l_crtc_state(rcrtc->crtc.state);
> >  	struct rzg2l_du_device *rcdu = rcrtc->dev;
> > +	u32 val = DU_MCR0_DI_EN;
> >
> > -	writel(start ? DU_MCR0_DI_EN : 0, rcdu->mmio + DU_MCR0);
> > +	if (rstate->outputs == BIT(RZG2L_DU_OUTPUT_DPAD0))
> 
> Is output supposed to contain a single bit, or can it contain multiple bits ? In the first case I
> would rename it to output, in the second case you should probably use '&' instead of '=='.

It may contain multiple bits for the future SoCs. So, I will use '&' instead.

> 
> > +		val |= DU_MCR0_DPI_OE;
> > +
> > +	writel(start ? val : 0, rcdu->mmio + DU_MCR0);
> >  }
> >
> >  static void rzg2l_du_crtc_start(struct rzg2l_du_crtc *rcrtc) diff
> > --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> > b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> > index e5eca8691a33..69b8e216ee1a 100644
> > --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> > @@ -25,21 +25,35 @@
> >   * Device Information
> >   */
> >
> > +static const struct rzg2l_du_device_info rzg2l_du_r9a07g043u_info = {
> > +	.channels_mask = BIT(0),
> > +	.routes = {
> > +		[RZG2L_DU_OUTPUT_DSI0] = {
> > +			.du_output = RZG2L_DU_OUTPUT_INVALID,
> > +		},
> 
> You can drop this entry, as well as the RZG2L_DU_OUTPUT_INVALID macro.
> See below.
OK.
> 
> > +		[RZG2L_DU_OUTPUT_DPAD0] = {
> > +			.possible_outputs = BIT(0),
> > +			.du_output = RZG2L_DU_OUTPUT_DPAD0,
> > +		},
> > +	},
> > +};
> > +
> >  static const struct rzg2l_du_device_info rzg2l_du_r9a07g044_info = {
> >  	.channels_mask = BIT(0),
> >  	.routes = {
> >  		[RZG2L_DU_OUTPUT_DSI0] = {
> >  			.possible_outputs = BIT(0),
> > -			.port = 0,
> > +			.du_output = RZG2L_DU_OUTPUT_DSI0,
> >  		},
> >  		[RZG2L_DU_OUTPUT_DPAD0] = {
> >  			.possible_outputs = BIT(0),
> > -			.port = 1,
> > +			.du_output = RZG2L_DU_OUTPUT_DPAD0,
> >  		}
> >  	}
> >  };
> >
> >  static const struct of_device_id rzg2l_du_of_table[] = {
> > +	{ .compatible = "renesas,r9a07g043u-du", .data =
> > +&rzg2l_du_r9a07g043u_info },
> >  	{ .compatible = "renesas,r9a07g044-du", .data = &rzg2l_du_r9a07g044_info },
> >  	{ /* sentinel */ }
> >  };
> > diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h
> > b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h
> > index 58806c2a8f2b..ab82b5c86d6e 100644
> > --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h
> > @@ -29,7 +29,7 @@ enum rzg2l_du_output {
> >  /*
> >   * struct rzg2l_du_output_routing - Output routing specification
> >   * @possible_outputs: bitmask of possible outputs
> > - * @port: device tree port number corresponding to this output route
> > + * @du_output: DU output
> >   *
> >   * The DU has 2 possible outputs (DPAD0, DSI0). Output routing data
> >   * specify the valid SoC outputs, which CRTC can drive the output,
> > and the type @@ -37,7 +37,7 @@ enum rzg2l_du_output {
> >   */
> >  struct rzg2l_du_output_routing {
> >  	unsigned int possible_outputs;
> > -	unsigned int port;
> > +	unsigned int du_output;
> >  };
> >
> >  /*
> > @@ -53,6 +53,7 @@ struct rzg2l_du_device_info {
> >  #define RZG2L_DU_MAX_CRTCS		1
> >  #define RZG2L_DU_MAX_VSPS		1
> >  #define RZG2L_DU_MAX_DSI		1
> > +#define RZG2L_DU_OUTPUT_INVALID		-1
> >
> >  struct rzg2l_du_device {
> >  	struct device *dev;
> > diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> > b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> > index 07b312b6f81e..361350f2999e 100644
> > --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> > @@ -183,8 +183,8 @@ static int rzg2l_du_encoders_init(struct
> > rzg2l_du_device *rcdu)
> >
> >  		/* Find the output route corresponding to the port number. */
> >  		for (i = 0; i < RZG2L_DU_OUTPUT_MAX; ++i) {
> > -			if (rcdu->info->routes[i].port == ep.port) {
> > -				output = i;
> > +			if (i == rcdu->info->routes[i].du_output) {
> 
> If I understand the code correctly, this will always be true except for the routes marked with
> RZG2L_DU_OUTPUT_INVALID, so you will match the first valid route, regardless of the value of ep.port.
> I don't think that's correct.
> 
> I would keep the port field in the rzg2l_du_output_routing, drop the newly added du_output field, and
> use the following logic:
> 
> 		for (i = 0; i < RZG2L_DU_OUTPUT_MAX; ++i) {
> 			if (rcdu->info->routes[i].possible_outputs &&
> 			    rcdu->info->routes[i].port == ep.port) {
> 				output = i;
> 				break;
> 			}
> 		}
> 
> Testing possible_outputs skips the routes that don't exist for the device, and the ep.port comparison
> picks the route corresponding to the port.

Thanks, this will work.

Cheers,
Biju

> 
> > +				output = rcdu->info->routes[i].du_output;
> >  				break;
> >  			}
> >  		}
> 
> --
> Regards,
> 
> Laurent Pinchart

RE: [PATCH v3 1/4] dt-bindings: display: renesas,rzg2l-du: Document RZ/G2UL DU bindings

[Biju Das]

Hi Rob and all,

> -----Original Message-----
> From: Biju Das
> Sent: Monday, August 19, 2024 1:37 PM
> Subject: RE: [PATCH v3 1/4] dt-bindings: display: renesas,rzg2l-du: Document RZ/G2UL DU bindings
> 
> Hi Laurent and Rob,
> 
> Thanks for the feedback
> 
> > -----Original Message-----
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Sent: Tuesday, August 13, 2024 8:39 PM
> > Subject: Re: [PATCH v3 1/4] dt-bindings: display: renesas,rzg2l-du:
> > Document RZ/G2UL DU bindings
> >
> > Hi Rob,
> >
> > On Tue, Aug 13, 2024 at 10:32:20AM -0600, Rob Herring wrote:
> > > On Mon, Aug 05, 2024 at 04:52:35PM +0100, Biju Das wrote:
> > > > Document DU found in RZ/G2UL SoC. The DU block is identical to
> > > > RZ/G2L SoC, but has only DPI interface.
> > > >
> > > > While at it, add missing required property port@1 for RZ/G2L and
> > > > RZ/V2L SoCs. Currently there is no user for the DPI interface and
> > > > hence there won't be any ABI breakage for adding port@1 as
> > > > required property for RZ/G2L and RZ/V2L SoCs.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > ---
> > > > v2->v3:
> > > >  * Replaced ports->port property for RZ/G2UL as it supports only DPI.
> > > >    and retained ports property for RZ/{G2L,V2L} as it supports both DSI
> > > >    and DPI output interface.
> > >
> > > Why? Having port and ports is just a needless complication.
> >
> > I agree that making the ports node mandatory, even when the device has
> > a single port, will simplify the bindings. In hindsight we should never have made ports optional,
> but that can't be changed.
> >
> > > >  * Added missing blank line before example.
> > > >  * Dropped tags from Conor and Geert as there are new changes.
> > > > v1->v2:
> > > >  * Updated commit description related to non ABI breakage.
> > > >  * Added Ack from Conor.
> > > > ---
> > > >  .../bindings/display/renesas,rzg2l-du.yaml    | 35 +++++++++++++++++--
> > > >  1 file changed, 32 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > > > b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > > > index 08e5b9478051..ca01bf26c4c0 100644
> > > > ---
> > > > a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > > > +++ b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.y
> > > > +++ am
> > > > +++ l
> > > > @@ -18,6 +18,7 @@ properties:
> > > >    compatible:
> > > >      oneOf:
> > > >        - enum:
> > > > +          - renesas,r9a07g043u-du # RZ/G2UL
> > > >            - renesas,r9a07g044-du # RZ/G2{L,LC}
> > > >        - items:
> > > >            - enum:
> > > > @@ -60,8 +61,9 @@ properties:
> > > >          $ref: /schemas/graph.yaml#/properties/port
> > > >          unevaluatedProperties: false
> > > >
> > > > -    required:
> > > > -      - port@0
> > > > +  port:
> > > > +    $ref: /schemas/graph.yaml#/properties/port
> > > > +    description: Connection to the DU output video port.
> > > >
> > > >      unevaluatedProperties: false
> > > >
> > > > @@ -83,11 +85,38 @@ required:
> > > >    - clock-names
> > > >    - resets
> > > >    - power-domains
> > > > -  - ports
> > > >    - renesas,vsps
> > > >
> > > >  additionalProperties: false
> > > >
> > > > +allOf:
> > > > +  - if:
> > > > +      properties:
> > > > +        compatible:
> > > > +          contains:
> > > > +            const: renesas,r9a07g043u-du
> > > > +    then:
> > > > +      properties:
> > > > +        port:
> > > > +          description: DPI
> > >
> > > This is equivalent to 'port@0'. IMO, this case should have a 'port@1'
> > > node so that DPI interface is *always* the same port.
> >
> > That's what Biju did in the previous version, and I recommended to
> > number the ports based on hardware indices, not types. Mapping port
> > numbers to the hardware documentation makes it more consistent for DT
> > writers, makes the logic simpler to understand (in my opinion, based
> > on my experience with the R-Car
> > DU) on the driver side, but most importantly, type-based numbering
> > wouldn't scale as SoCs could have multiple ports of the same type (we've seen that happening with R-
> Car).
> 
> OK, I will send bindings based on hardware indices.

I get a warning, if I use ports and port@0 for single port. I don't see this warning if I use ports and port@1

arch/arm64/boot/dts/renesas/r9a07g043u.dtsi:169.9-178.5: Warning (graph_child_address): /soc/display@10890000/ports: graph node has single child node 'port@0', #address-cells/#size-cells are not necessary

Can we fix the dt-schema to getting consistent results for single port usage involving ports?


+  - if:
+      properties:
+        compatible:
+          contains:
+            const: renesas,r9a07g043u-du
+    then:
+      properties:
+        ports:
+          properties:
+            port@0:
+              description: DPI
+
+          required:
+            - port@0

Cheers,
Biju

Re: [PATCH v3 1/4] dt-bindings: display: renesas,rzg2l-du: Document RZ/G2UL DU bindings

[Laurent Pinchart]

On Tue, Aug 20, 2024 at 06:42:48AM +0000, Biju Das wrote:
> Hi Rob and all,
> 
> > -----Original Message-----
> > From: Biju Das
> > Sent: Monday, August 19, 2024 1:37 PM
> > Subject: RE: [PATCH v3 1/4] dt-bindings: display: renesas,rzg2l-du: Document RZ/G2UL DU bindings
> > 
> > Hi Laurent and Rob,
> > 
> > Thanks for the feedback
> > 
> > > -----Original Message-----
> > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Sent: Tuesday, August 13, 2024 8:39 PM
> > > Subject: Re: [PATCH v3 1/4] dt-bindings: display: renesas,rzg2l-du:
> > > Document RZ/G2UL DU bindings
> > >
> > > Hi Rob,
> > >
> > > On Tue, Aug 13, 2024 at 10:32:20AM -0600, Rob Herring wrote:
> > > > On Mon, Aug 05, 2024 at 04:52:35PM +0100, Biju Das wrote:
> > > > > Document DU found in RZ/G2UL SoC. The DU block is identical to
> > > > > RZ/G2L SoC, but has only DPI interface.
> > > > >
> > > > > While at it, add missing required property port@1 for RZ/G2L and
> > > > > RZ/V2L SoCs. Currently there is no user for the DPI interface and
> > > > > hence there won't be any ABI breakage for adding port@1 as
> > > > > required property for RZ/G2L and RZ/V2L SoCs.
> > > > >
> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > ---
> > > > > v2->v3:
> > > > >  * Replaced ports->port property for RZ/G2UL as it supports only DPI.
> > > > >    and retained ports property for RZ/{G2L,V2L} as it supports both DSI
> > > > >    and DPI output interface.
> > > >
> > > > Why? Having port and ports is just a needless complication.
> > >
> > > I agree that making the ports node mandatory, even when the device has
> > > a single port, will simplify the bindings. In hindsight we should never have made ports optional,
> > but that can't be changed.
> > >
> > > > >  * Added missing blank line before example.
> > > > >  * Dropped tags from Conor and Geert as there are new changes.
> > > > > v1->v2:
> > > > >  * Updated commit description related to non ABI breakage.
> > > > >  * Added Ack from Conor.
> > > > > ---
> > > > >  .../bindings/display/renesas,rzg2l-du.yaml    | 35 +++++++++++++++++--
> > > > >  1 file changed, 32 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > > > > b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > > > > index 08e5b9478051..ca01bf26c4c0 100644
> > > > > ---
> > > > > a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > > > > +++ b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.y
> > > > > +++ am
> > > > > +++ l
> > > > > @@ -18,6 +18,7 @@ properties:
> > > > >    compatible:
> > > > >      oneOf:
> > > > >        - enum:
> > > > > +          - renesas,r9a07g043u-du # RZ/G2UL
> > > > >            - renesas,r9a07g044-du # RZ/G2{L,LC}
> > > > >        - items:
> > > > >            - enum:
> > > > > @@ -60,8 +61,9 @@ properties:
> > > > >          $ref: /schemas/graph.yaml#/properties/port
> > > > >          unevaluatedProperties: false
> > > > >
> > > > > -    required:
> > > > > -      - port@0
> > > > > +  port:
> > > > > +    $ref: /schemas/graph.yaml#/properties/port
> > > > > +    description: Connection to the DU output video port.
> > > > >
> > > > >      unevaluatedProperties: false
> > > > >
> > > > > @@ -83,11 +85,38 @@ required:
> > > > >    - clock-names
> > > > >    - resets
> > > > >    - power-domains
> > > > > -  - ports
> > > > >    - renesas,vsps
> > > > >
> > > > >  additionalProperties: false
> > > > >
> > > > > +allOf:
> > > > > +  - if:
> > > > > +      properties:
> > > > > +        compatible:
> > > > > +          contains:
> > > > > +            const: renesas,r9a07g043u-du
> > > > > +    then:
> > > > > +      properties:
> > > > > +        port:
> > > > > +          description: DPI
> > > >
> > > > This is equivalent to 'port@0'. IMO, this case should have a 'port@1'
> > > > node so that DPI interface is *always* the same port.
> > >
> > > That's what Biju did in the previous version, and I recommended to
> > > number the ports based on hardware indices, not types. Mapping port
> > > numbers to the hardware documentation makes it more consistent for DT
> > > writers, makes the logic simpler to understand (in my opinion, based
> > > on my experience with the R-Car
> > > DU) on the driver side, but most importantly, type-based numbering
> > > wouldn't scale as SoCs could have multiple ports of the same type
> > > (we've seen that happening with R-Car).
> > 
> > OK, I will send bindings based on hardware indices.
> 
> I get a warning, if I use ports and port@0 for single port. I don't
> see this warning if I use ports and port@1
> 
> arch/arm64/boot/dts/renesas/r9a07g043u.dtsi:169.9-178.5: Warning (graph_child_address): /soc/display@10890000/ports: graph node has single child node 'port@0', #address-cells/#size-cells are not necessary
> 
> Can we fix the dt-schema to getting consistent results for single port
> usage involving ports?

I would like that too.

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: renesas,r9a07g043u-du
> +    then:
> +      properties:
> +        ports:
> +          properties:
> +            port@0:
> +              description: DPI
> +
> +          required:
> +            - port@0

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 2/4] drm: renesas: rz-du: Add RZ/G2UL DU Support

[Geert Uytterhoeven]

Hi Biju,

On Mon, Aug 5, 2024 at 6:22 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> The LCD controller is composed of Frame Compression Processor (FCPVD),
> Video Signal Processor (VSPD), and Display Unit (DU).
>
> It has DPI interface and supports a maximum resolution of WXGA along
> with 2 RPFs to support the blending of two picture layers and raster
> operations (ROPs).
>
> The DU module is connected to VSPD. Add RZ/G2UL DU support.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2->v3:
>  * Avoided the line break in rzg2l_du_start_stop() for rstate.
>  * Replaced port->du_output in  struct rzg2l_du_output_routing and
>    dropped using the port number to indicate the output type in
>    rzg2l_du_encoders_init().
>  * Updated rzg2l_du_r9a07g043u_info and rzg2l_du_r9a07g044_info

Thanks for your patch!

> --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> @@ -183,8 +183,8 @@ static int rzg2l_du_encoders_init(struct rzg2l_du_device *rcdu)
>
>                 /* Find the output route corresponding to the port number. */
>                 for (i = 0; i < RZG2L_DU_OUTPUT_MAX; ++i) {
> -                       if (rcdu->info->routes[i].port == ep.port) {
> -                               output = i;
> +                       if (i == rcdu->info->routes[i].du_output) {
> +                               output = rcdu->info->routes[i].du_output;

Notwithstanding Laurent's comment, the above replacement is equivalent
to the original "output = i;", so there is no need to change this line.

>                                 break;
>                         }
>                 }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v3 2/4] drm: renesas: rz-du: Add RZ/G2UL DU Support

[Biju Das]

Hi Geert Uytterhoeven,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Thursday, August 22, 2024 1:31 PM
> Subject: Re: [PATCH v3 2/4] drm: renesas: rz-du: Add RZ/G2UL DU Support
> 
> Hi Biju,
> 
> On Mon, Aug 5, 2024 at 6:22 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > The LCD controller is composed of Frame Compression Processor (FCPVD),
> > Video Signal Processor (VSPD), and Display Unit (DU).
> >
> > It has DPI interface and supports a maximum resolution of WXGA along
> > with 2 RPFs to support the blending of two picture layers and raster
> > operations (ROPs).
> >
> > The DU module is connected to VSPD. Add RZ/G2UL DU support.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v2->v3:
> >  * Avoided the line break in rzg2l_du_start_stop() for rstate.
> >  * Replaced port->du_output in  struct rzg2l_du_output_routing and
> >    dropped using the port number to indicate the output type in
> >    rzg2l_du_encoders_init().
> >  * Updated rzg2l_du_r9a07g043u_info and rzg2l_du_r9a07g044_info
> 
> Thanks for your patch!
> 
> > --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> > @@ -183,8 +183,8 @@ static int rzg2l_du_encoders_init(struct
> > rzg2l_du_device *rcdu)
> >
> >                 /* Find the output route corresponding to the port number. */
> >                 for (i = 0; i < RZG2L_DU_OUTPUT_MAX; ++i) {
> > -                       if (rcdu->info->routes[i].port == ep.port) {
> > -                               output = i;
> > +                       if (i == rcdu->info->routes[i].du_output) {
> > +                               output =
> > + rcdu->info->routes[i].du_output;
> 
> Notwithstanding Laurent's comment, the above replacement is equivalent to the original "output = i;",
> so there is no need to change this line.

Agreed. I missed that.

Cheers,
Biju

> 
> >                                 break;
> >                         }
> >                 }
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But when I'm talking to
> journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds