[PATCH v7 0/3] Add support for RZ/G2L VSPD

[PATCH v7 0/3] Add support for RZ/G2L VSPD

[Biju Das]

The RZ/G2L VSPD provides a single VSPD instance. It has the following
sub modules MAU, CTU, RPF, DPR, LUT, BRS, WPF and LIF.

The VSPD block on RZ/G2L does not have a version register, so added a
new compatible string "renesas,rzg2l-vsp2" with a data pointer containing
the info structure. Also the reset line is shared with the DU module.

v6->v7:
 * Added Rb tag from Kieran for patch#3
 * Added a quirk to handle LIF0 buffer attribute related
   changes for V3M and G2L.
 * Removed the macro for VSP HW version
v5->v6:
 * Rebased to media_staging and updated commit header
 * Removed LCDC reference clock description from bindings
 * Changed the clock name from du.0->aclk from bindings
 * Added Rb tag from Laurent for reset patch
 * Added forward declaration for struct reset_control
 * Updated vsp1_device_get() with changes suggested by Laurent
 * Updated error message for reset_control_get form ctrl->control.
 * Removed the extra tab from rzg2l_vsp2_device_info
 * Changed the function vsp1_lookup->vsp1_lookup_info and
   all info match related code moved here.
 * Add VI6_IP_VERSION_VSP and VI6_IP_VERSION_VSP_SW macros to
   distinguish HW & SW IP_VSP_Version.
 * Used 0x80 for RZG2L VSPD model and SoC identification
 * Updated Switch() for LIF0 buffer attribute handling.
v4->v5:
 * Fixed typo VI6_IP_VERSION_MODEL_MASK->VI6_IP_VERSION_MASK
 * To be consistent with other SoC's, introduced VI6_IP_VERSION_SOC_G2L
   for SoC identification for RZ/G2L SoC's.
v3->v4:
 * Restored error check for pm_runtime_resume_and_get and calls
   assert() in case of failure.
 * Added Rb tag from Geert
 * Add switch() for LIF0 buffer attribute handling for RZ/G2L and V3M SoC's
v2->v3:
 * Added Rb tags from Krzysztof and Philipp
 * If reset_control_deassert() failed, return ret directly.
 * Fixed version comparison in vsp1_lookup()
v1->v2:
 * Used reference counted reset handle to perform deassert/assert
 * Changed the compatible from vsp2-rzg2l->rzg2l-vsp2
 * Added standalone device info for rzg2l-vsp2.
 * Added vsp1_lookup helper function.
 * Updated comments for LIF0 buffer attribute register
 * Used last ID for rzg2l-vsp2.
RFC->v1:
 * Added reset support as separate patch
 * Moved rstc just after the bus_master field in struct vsp1_device
 * Used data pointer containing info structure to retrieve version information
 * Updated commit description
 * Changed compatible from vsp2-r9a07g044->vsp2-rzg2l
 * Defined the clocks
 * Clock max Items is based on SoC Compatible string

RFC:
 * https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-21-biju.das.jz@bp.renesas.com/
 * https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-20-biju.das.jz@bp.renesas.com/

Biju Das (3):
  media: dt-bindings: media: renesas,vsp1: Document RZ/{G2L,V2L} VSPD
    bindings
  media: renesas: vsp1: Add support to deassert/assert reset line
  media: renesas: vsp1: Add support for RZ/G2L VSPD

 .../bindings/media/renesas,vsp1.yaml          | 52 +++++++++---
 drivers/media/platform/renesas/vsp1/vsp1.h    |  5 ++
 .../media/platform/renesas/vsp1/vsp1_drv.c    | 83 +++++++++++++++----
 .../media/platform/renesas/vsp1/vsp1_lif.c    | 13 +--
 .../media/platform/renesas/vsp1/vsp1_lif.h    |  1 +
 .../media/platform/renesas/vsp1/vsp1_regs.h   |  7 ++
 6 files changed, 126 insertions(+), 35 deletions(-)

-- 
2.25.1

[PATCH v7 1/3] media: dt-bindings: media: renesas,vsp1: Document RZ/{G2L,V2L} VSPD bindings To: Mauro Carvalho Chehab <mchehab@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>

[Biju Das]

Document VSPD found in RZ/G2L and RZ/V2L family SoC's. VSPD block is
similar to VSP2-D found on R-Car SoC's, but it does not have a version
register and it has 3 clocks compared to 1 clock on vsp1 and vsp2.

This patch introduces a new compatible 'renesas,rzg2l-vsp2' to handle
these differences.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
v6->v7:
 * No change
v5->v6:
 * Removed LCDC reference clock description
 * Changed the clock name from du.0->aclk
v4->v5:
 * No change
v3->v4:
 * No change
v2->v3:
 * Added Rb tag from Krzysztof.
v1->v2:
 * Changed compatible from vsp2-rzg2l->rzg2l-vsp2
RFC->v1:
 * Updated commit description
 * Changed compatible from vsp2-r9a07g044->vsp2-rzg2l
 * Defined the clocks
 * Clock max Items is based on SoC Compatible string
RFC:
 * https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-20-biju.das.jz@bp.renesas.com/
---
 .../bindings/media/renesas,vsp1.yaml          | 52 ++++++++++++++-----
 1 file changed, 39 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
index 990e9c1dbc43..a236b266fa4b 100644
--- a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
+++ b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
@@ -19,6 +19,7 @@ properties:
     enum:
       - renesas,vsp1 # R-Car Gen2 and RZ/G1
       - renesas,vsp2 # R-Car Gen3 and RZ/G2
+      - renesas,rzg2l-vsp2 # RZ/G2L and RZ/V2L
 
   reg:
     maxItems: 1
@@ -26,8 +27,8 @@ properties:
   interrupts:
     maxItems: 1
 
-  clocks:
-    maxItems: 1
+  clocks: true
+  clock-names: true
 
   power-domains:
     maxItems: 1
@@ -50,17 +51,42 @@ required:
 
 additionalProperties: false
 
-if:
-  properties:
-    compatible:
-      items:
-        - const: renesas,vsp1
-then:
-  properties:
-    renesas,fcp: false
-else:
-  required:
-    - renesas,fcp
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: renesas,vsp1
+    then:
+      properties:
+        renesas,fcp: false
+    else:
+      required:
+        - renesas,fcp
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: renesas,rzg2l-vsp2
+    then:
+      properties:
+        clocks:
+          items:
+            - description: Main clock
+            - description: Register access clock
+            - description: Video clock
+        clock-names:
+          items:
+            - const: aclk
+            - const: pclk
+            - const: vclk
+      required:
+        - clock-names
+    else:
+      properties:
+        clocks:
+          maxItems: 1
 
 examples:
   # R8A7790 (R-Car H2) VSP1-S
-- 
2.25.1

[PATCH v7 2/3] media: renesas: vsp1: Add support to deassert/assert reset line

[Biju Das]

As the resets DT property is mandatory, and is present in all .dtsi
in mainline, add support to perform deassert/assert using reference
counted reset handle.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
v6->v7:
 * No change
v5->v6:
 * Rebased to media_staging and updated commit header
 * Added Rb tag from Laurent
 * Added forward declaration for struct reset_control
 * Updated vsp1_device_get() with changes suggested by Laurent
 * Updated error message for reset_control_get form ctrl->control.
v4->v5:
 * Added Rb tag from Geert
v3->v4:
 * Restored error check for pm_runtime_resume_and_get and calls
   assert() in case of failure.
v2->v3:
 * Added Rb tag from Philipp
 * If reset_control_deassert() failed, return ret directly. 
v1->v2:
 * Used reference counted reset handle to perform deassert/assert
RFC->v1:
 * Added reset support as separate patch
 * Moved rstc just after the bus_master field in struct vsp1_device
RFC:
 * https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-21-biju.das.jz@bp.renesas.com/
---
 drivers/media/platform/renesas/vsp1/vsp1.h    |  2 ++
 .../media/platform/renesas/vsp1/vsp1_drv.c    | 19 ++++++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/renesas/vsp1/vsp1.h b/drivers/media/platform/renesas/vsp1/vsp1.h
index 37cf33c7e6ca..baf898d577ec 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1.h
+++ b/drivers/media/platform/renesas/vsp1/vsp1.h
@@ -22,6 +22,7 @@
 struct clk;
 struct device;
 struct rcar_fcp_device;
+struct reset_control;
 
 struct vsp1_drm;
 struct vsp1_entity;
@@ -79,6 +80,7 @@ struct vsp1_device {
 	void __iomem *mmio;
 	struct rcar_fcp_device *fcp;
 	struct device *bus_master;
+	struct reset_control *rstc;
 
 	struct vsp1_brx *brs;
 	struct vsp1_brx *bru;
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
index 502c7d9d6890..159b68fa0829 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
@@ -16,6 +16,7 @@
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 #include <linux/videodev2.h>
 
 #include <media/rcar-fcp.h>
@@ -569,7 +570,17 @@ static void vsp1_mask_all_interrupts(struct vsp1_device *vsp1)
  */
 int vsp1_device_get(struct vsp1_device *vsp1)
 {
-	return pm_runtime_resume_and_get(vsp1->dev);
+	int ret;
+
+	ret = reset_control_deassert(vsp1->rstc);
+	if (ret < 0)
+		return ret;
+
+	ret = pm_runtime_resume_and_get(vsp1->dev);
+	if (ret < 0)
+		reset_control_assert(vsp1->rstc);
+
+	return ret;
 }
 
 /*
@@ -581,6 +592,7 @@ int vsp1_device_get(struct vsp1_device *vsp1)
 void vsp1_device_put(struct vsp1_device *vsp1)
 {
 	pm_runtime_put_sync(vsp1->dev);
+	reset_control_assert(vsp1->rstc);
 }
 
 /* -----------------------------------------------------------------------------
@@ -827,6 +839,11 @@ static int vsp1_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
+	vsp1->rstc = devm_reset_control_get_shared(&pdev->dev, NULL);
+	if (IS_ERR(vsp1->rstc))
+		return dev_err_probe(&pdev->dev, PTR_ERR(vsp1->rstc),
+				     "failed to get reset control\n");
+
 	/* FCP (optional). */
 	fcp_node = of_parse_phandle(pdev->dev.of_node, "renesas,fcp", 0);
 	if (fcp_node) {
-- 
2.25.1

[PATCH v7 3/3] media: renesas: vsp1: Add support for RZ/G2L VSPD

[Biju Das]

The RZ/G2L VSPD provides a single VSPD instance. It has the following
sub modules MAU, CTU, RPF, DPR, LUT, BRS, WPF and LIF.

The VSPD block on RZ/G2L does not have a version register, so added a
new compatible string "renesas,rzg2l-vsp2" with a data pointer containing
the info structure. Also the reset line is shared with the DU module.

Apart from this, added a quirk to handle LIF0 buffer attribute to fix
the artifacts on the output for both R-Car V3M and RZ/G2L alike SoC's.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
v6->v7:
 * Added Rb tag from Kieran
 * Added a quirk to handle LIF0 buffer attribute related
   changes for V3M and G2L.
 * Removed the macro for VSP HW version
v5->v6:
 * Rebased to media_staging and updated commit header
 * Removed the extra tab from rzg2l_vsp2_device_info
 * Changed the function vsp1_lookup->vsp1_lookup_info and
   all info match related code moved here.
 * Add VI6_IP_VERSION_VSP and VI6_IP_VERSION_VSP_SW macros to
   distinguish HW & SW IP_VSP_Version.
 * Used 0x80 for RZG2L VSPD model and SoC identification
 * Updated Switch() for LIF0 buffer attribute handling.
v4->v5:
 * Fixed typo VI6_IP_VERSION_MODEL_MASK->VI6_IP_VERSION_MASK
 * To be consistent with other SoC's, introduced VI6_IP_VERSION_SOC_G2L
   for RZ/G2L SoC's.
v3->v4:
 * Added Rb tag from Geert
 * Add switch() for LIF0 buffer attribute handling for RZ/G2L and V3M
v2->v3:
 * Fixed version comparison in vsp1_lookup()
v1->v2:
 * Changed the compatible from vsp2-rzg2l->rzg2l-vsp2
 * Added standalone device info for rzg2l-vsp2.
 * Added vsp1_lookup helper function.
 * Updated comments for LIF0 buffer attribute register
 * Used last ID for rzg2l-vsp2.
RFC->v1:
 * Used data pointer containing info structure to retrieve version information
RFC:
 * https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-21-biju.das.jz@bp.renesas.com/
---
 drivers/media/platform/renesas/vsp1/vsp1.h    |  3 +
 .../media/platform/renesas/vsp1/vsp1_drv.c    | 64 ++++++++++++++-----
 .../media/platform/renesas/vsp1/vsp1_lif.c    | 13 ++--
 .../media/platform/renesas/vsp1/vsp1_lif.h    |  1 +
 .../media/platform/renesas/vsp1/vsp1_regs.h   |  7 ++
 5 files changed, 67 insertions(+), 21 deletions(-)

diff --git a/drivers/media/platform/renesas/vsp1/vsp1.h b/drivers/media/platform/renesas/vsp1/vsp1.h
index baf898d577ec..de3685cc89f3 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1.h
+++ b/drivers/media/platform/renesas/vsp1/vsp1.h
@@ -56,6 +56,8 @@ struct vsp1_uif;
 #define VSP1_HAS_BRS		BIT(9)
 #define VSP1_HAS_EXT_DL		BIT(10)
 
+#define LIF_BUF_ATTR_QUIRK	BIT(0)
+
 struct vsp1_device_info {
 	u32 version;
 	const char *model;
@@ -76,6 +78,7 @@ struct vsp1_device {
 	struct device *dev;
 	const struct vsp1_device_info *info;
 	u32 version;
+	u32 quirks;
 
 	void __iomem *mmio;
 	struct rcar_fcp_device *fcp;
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
index 159b68fa0829..206fe18c3c09 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
@@ -812,11 +812,57 @@ static const struct vsp1_device_info vsp1_device_infos[] = {
 	},
 };
 
+static const struct vsp1_device_info rzg2l_vsp2_device_info = {
+	.version = VI6_IP_VERSION_MODEL_VSPD_RZG2L,
+	.model = "VSP2-D",
+	.gen = 3,
+	.features = VSP1_HAS_BRS | VSP1_HAS_WPF_VFLIP | VSP1_HAS_EXT_DL,
+	.lif_count = 1,
+	.rpf_count = 2,
+	.wpf_count = 1,
+};
+
+static const struct vsp1_device_info *vsp1_lookup_info(struct vsp1_device *vsp1)
+{
+	const struct vsp1_device_info *info;
+	unsigned int i;
+
+	/*
+	 * Try the info stored in match data first for devices that don't have
+	 * a version register.
+	 */
+	info = of_device_get_match_data(vsp1->dev);
+	if (info) {
+		vsp1->quirks = LIF_BUF_ATTR_QUIRK;
+		vsp1->version = VI6_IP_VERSION_VSP_SW | info->version |
+				VI6_IP_VERSION_SOC_RZG2L;
+
+		return info;
+	}
+
+	vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
+
+	for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
+		info = &vsp1_device_infos[i];
+
+		if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) == info->version) {
+			if ((vsp1->version & VI6_IP_VERSION_MASK) ==
+			    (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M))
+				vsp1->quirks = LIF_BUF_ATTR_QUIRK;
+
+			return info;
+		}
+	}
+
+	dev_err(vsp1->dev, "unsupported IP version 0x%08x\n", vsp1->version);
+
+	return NULL;
+}
+
 static int vsp1_probe(struct platform_device *pdev)
 {
 	struct vsp1_device *vsp1;
 	struct device_node *fcp_node;
-	unsigned int i;
 	int ret;
 	int irq;
 
@@ -872,26 +918,13 @@ static int vsp1_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto done;
 
-	vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
-
-	for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
-		if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) ==
-		    vsp1_device_infos[i].version) {
-			vsp1->info = &vsp1_device_infos[i];
-			break;
-		}
-	}
-
+	vsp1->info = vsp1_lookup_info(vsp1);
 	if (!vsp1->info) {
-		dev_err(&pdev->dev, "unsupported IP version 0x%08x\n",
-			vsp1->version);
 		vsp1_device_put(vsp1);
 		ret = -ENXIO;
 		goto done;
 	}
 
-	dev_dbg(&pdev->dev, "IP version 0x%08x\n", vsp1->version);
-
 	/*
 	 * Previous use of the hardware (e.g. by the bootloader) could leave
 	 * some interrupts enabled and pending.
@@ -941,6 +974,7 @@ static int vsp1_remove(struct platform_device *pdev)
 static const struct of_device_id vsp1_of_match[] = {
 	{ .compatible = "renesas,vsp1" },
 	{ .compatible = "renesas,vsp2" },
+	{ .compatible = "renesas,rzg2l-vsp2", .data = &rzg2l_vsp2_device_info },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, vsp1_of_match);
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_lif.c b/drivers/media/platform/renesas/vsp1/vsp1_lif.c
index 6a6857ac9327..e041871185d2 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_lif.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_lif.c
@@ -107,6 +107,7 @@ static void lif_configure_stream(struct vsp1_entity *entity,
 
 	case VI6_IP_VERSION_MODEL_VSPDL_GEN3:
 	case VI6_IP_VERSION_MODEL_VSPD_V3:
+	case VI6_IP_VERSION_MODEL_VSPD_RZG2L:
 		hbth = 0;
 		obth = 1500;
 		lbth = 0;
@@ -130,13 +131,12 @@ static void lif_configure_stream(struct vsp1_entity *entity,
 			VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN);
 
 	/*
-	 * On R-Car V3M the LIF0 buffer attribute register has to be set to a
-	 * non-default value to guarantee proper operation (otherwise artifacts
-	 * may appear on the output). The value required by the manual is not
-	 * explained but is likely a buffer size or threshold.
+	 * On R-Car V3M and RZ/G2L the LIF0 buffer attribute register has to be
+	 * set to a non-default value to guarantee proper operation (otherwise
+	 * artifacts may appear on the output). The value required by the
+	 * manual is not explained but is likely a buffer size or threshold.
 	 */
-	if ((entity->vsp1->version & VI6_IP_VERSION_MASK) ==
-	    (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M))
+	if (lif->quirks)
 		vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
 			       VI6_LIF_LBA_LBA0 |
 			       (1536 << VI6_LIF_LBA_LBA1_SHIFT));
@@ -162,6 +162,7 @@ struct vsp1_lif *vsp1_lif_create(struct vsp1_device *vsp1, unsigned int index)
 	lif->entity.ops = &lif_entity_ops;
 	lif->entity.type = VSP1_ENTITY_LIF;
 	lif->entity.index = index;
+	lif->quirks = vsp1->quirks & LIF_BUF_ATTR_QUIRK;
 
 	/*
 	 * The LIF is never exposed to userspace, but media entity registration
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_lif.h b/drivers/media/platform/renesas/vsp1/vsp1_lif.h
index 71a4eda9c2b2..f073784e429b 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_lif.h
+++ b/drivers/media/platform/renesas/vsp1/vsp1_lif.h
@@ -21,6 +21,7 @@ struct vsp1_device;
 
 struct vsp1_lif {
 	struct vsp1_entity entity;
+	u32 quirks;
 };
 
 static inline struct vsp1_lif *to_lif(struct v4l2_subdev *subdev)
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_regs.h b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
index fae7286eb01e..41bea15597c9 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_regs.h
+++ b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
@@ -767,6 +767,8 @@
 #define VI6_IP_VERSION_MODEL_VSPDL_GEN3	(0x19 << 8)
 #define VI6_IP_VERSION_MODEL_VSPBS_GEN3	(0x1a << 8)
 #define VI6_IP_VERSION_MODEL_VSPD_V3U	(0x1c << 8)
+/* RZ/G2L SoC's have no version register, So use 0x80 as the model version */
+#define VI6_IP_VERSION_MODEL_VSPD_RZG2L	(0x80 << 8)
 
 #define VI6_IP_VERSION_SOC_MASK		(0xff << 0)
 #define VI6_IP_VERSION_SOC_H2		(0x01 << 0)
@@ -780,6 +782,11 @@
 #define VI6_IP_VERSION_SOC_M3N		(0x04 << 0)
 #define VI6_IP_VERSION_SOC_E3		(0x04 << 0)
 #define VI6_IP_VERSION_SOC_V3U		(0x05 << 0)
+/* RZ/G2L SoC's have no version register, So use 0x80 for SoC Identification */
+#define VI6_IP_VERSION_SOC_RZG2L	(0x80 << 0)
+
+#define VI6_IP_VERSION_VSP_MASK		(0xffff << 16)
+#define VI6_IP_VERSION_VSP_SW		(0xfffe << 16) /* SW VSP version */
 
 /* -----------------------------------------------------------------------------
  * RPF CLUT Registers
-- 
2.25.1

Re: [PATCH v7 1/3] media: dt-bindings: media: renesas,vsp1: Document RZ/{G2L,V2L} VSPD bindings To: Mauro Carvalho Chehab <mchehab@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>

[Laurent Pinchart]

Hi Biju,

Thank you for the patch.

On Thu, Apr 14, 2022 at 03:26:03PM +0100, Biju Das wrote:
> Document VSPD found in RZ/G2L and RZ/V2L family SoC's. VSPD block is
> similar to VSP2-D found on R-Car SoC's, but it does not have a version
> register and it has 3 clocks compared to 1 clock on vsp1 and vsp2.
> 
> This patch introduces a new compatible 'renesas,rzg2l-vsp2' to handle
> these differences.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> ---
> v6->v7:
>  * No change
> v5->v6:
>  * Removed LCDC reference clock description
>  * Changed the clock name from du.0->aclk
> v4->v5:
>  * No change
> v3->v4:
>  * No change
> v2->v3:
>  * Added Rb tag from Krzysztof.
> v1->v2:
>  * Changed compatible from vsp2-rzg2l->rzg2l-vsp2
> RFC->v1:
>  * Updated commit description
>  * Changed compatible from vsp2-r9a07g044->vsp2-rzg2l
>  * Defined the clocks
>  * Clock max Items is based on SoC Compatible string
> RFC:
>  * https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-20-biju.das.jz@bp.renesas.com/
> ---
>  .../bindings/media/renesas,vsp1.yaml          | 52 ++++++++++++++-----
>  1 file changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> index 990e9c1dbc43..a236b266fa4b 100644
> --- a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> +++ b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> @@ -19,6 +19,7 @@ properties:
>      enum:
>        - renesas,vsp1 # R-Car Gen2 and RZ/G1
>        - renesas,vsp2 # R-Car Gen3 and RZ/G2
> +      - renesas,rzg2l-vsp2 # RZ/G2L and RZ/V2L
>  
>    reg:
>      maxItems: 1
> @@ -26,8 +27,8 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> -  clocks:
> -    maxItems: 1
> +  clocks: true
> +  clock-names: true

clock-names shouldn't be true here, as it should only be set on
rzg2l-vsp2. I think you can actually drop both clocks and clock-names
here.

With this addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>    power-domains:
>      maxItems: 1
> @@ -50,17 +51,42 @@ required:
>  
>  additionalProperties: false
>  
> -if:
> -  properties:
> -    compatible:
> -      items:
> -        - const: renesas,vsp1
> -then:
> -  properties:
> -    renesas,fcp: false
> -else:
> -  required:
> -    - renesas,fcp
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: renesas,vsp1
> +    then:
> +      properties:
> +        renesas,fcp: false
> +    else:
> +      required:
> +        - renesas,fcp
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: renesas,rzg2l-vsp2
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: Main clock
> +            - description: Register access clock
> +            - description: Video clock
> +        clock-names:
> +          items:
> +            - const: aclk
> +            - const: pclk
> +            - const: vclk
> +      required:
> +        - clock-names
> +    else:
> +      properties:
> +        clocks:
> +          maxItems: 1
>  
>  examples:
>    # R8A7790 (R-Car H2) VSP1-S

-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 3/3] media: renesas: vsp1: Add support for RZ/G2L VSPD

[Laurent Pinchart]

Hi Biju,

Thank you for the patch.

On Thu, Apr 14, 2022 at 03:26:05PM +0100, Biju Das wrote:
> The RZ/G2L VSPD provides a single VSPD instance. It has the following
> sub modules MAU, CTU, RPF, DPR, LUT, BRS, WPF and LIF.
> 
> The VSPD block on RZ/G2L does not have a version register, so added a
> new compatible string "renesas,rzg2l-vsp2" with a data pointer containing
> the info structure. Also the reset line is shared with the DU module.
> 
> Apart from this, added a quirk to handle LIF0 buffer attribute to fix
> the artifacts on the output for both R-Car V3M and RZ/G2L alike SoC's.

That's quite a few changes, it should have been split in multiple
patches (one that adds the software version mechanism, one that extends
the LIF buffer attribute handling mechanism, and one that adds RZ/G2L
support). You don't have to split the changes in the next version, but
please keep it in mind for future series.

> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> v6->v7:
>  * Added Rb tag from Kieran
>  * Added a quirk to handle LIF0 buffer attribute related
>    changes for V3M and G2L.
>  * Removed the macro for VSP HW version
> v5->v6:
>  * Rebased to media_staging and updated commit header
>  * Removed the extra tab from rzg2l_vsp2_device_info
>  * Changed the function vsp1_lookup->vsp1_lookup_info and
>    all info match related code moved here.
>  * Add VI6_IP_VERSION_VSP and VI6_IP_VERSION_VSP_SW macros to
>    distinguish HW & SW IP_VSP_Version.
>  * Used 0x80 for RZG2L VSPD model and SoC identification
>  * Updated Switch() for LIF0 buffer attribute handling.
> v4->v5:
>  * Fixed typo VI6_IP_VERSION_MODEL_MASK->VI6_IP_VERSION_MASK
>  * To be consistent with other SoC's, introduced VI6_IP_VERSION_SOC_G2L
>    for RZ/G2L SoC's.
> v3->v4:
>  * Added Rb tag from Geert
>  * Add switch() for LIF0 buffer attribute handling for RZ/G2L and V3M
> v2->v3:
>  * Fixed version comparison in vsp1_lookup()
> v1->v2:
>  * Changed the compatible from vsp2-rzg2l->rzg2l-vsp2
>  * Added standalone device info for rzg2l-vsp2.
>  * Added vsp1_lookup helper function.
>  * Updated comments for LIF0 buffer attribute register
>  * Used last ID for rzg2l-vsp2.
> RFC->v1:
>  * Used data pointer containing info structure to retrieve version information
> RFC:
>  * https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-21-biju.das.jz@bp.renesas.com/
> ---
>  drivers/media/platform/renesas/vsp1/vsp1.h    |  3 +
>  .../media/platform/renesas/vsp1/vsp1_drv.c    | 64 ++++++++++++++-----
>  .../media/platform/renesas/vsp1/vsp1_lif.c    | 13 ++--
>  .../media/platform/renesas/vsp1/vsp1_lif.h    |  1 +
>  .../media/platform/renesas/vsp1/vsp1_regs.h   |  7 ++
>  5 files changed, 67 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1.h b/drivers/media/platform/renesas/vsp1/vsp1.h
> index baf898d577ec..de3685cc89f3 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1.h
> +++ b/drivers/media/platform/renesas/vsp1/vsp1.h
> @@ -56,6 +56,8 @@ struct vsp1_uif;
>  #define VSP1_HAS_BRS		BIT(9)
>  #define VSP1_HAS_EXT_DL		BIT(10)
>  
> +#define LIF_BUF_ATTR_QUIRK	BIT(0)
> +
>  struct vsp1_device_info {
>  	u32 version;
>  	const char *model;
> @@ -76,6 +78,7 @@ struct vsp1_device {
>  	struct device *dev;
>  	const struct vsp1_device_info *info;
>  	u32 version;
> +	u32 quirks;

It would have been nice to make this a feature bit in the
vsp1_device_info structure instead, but that won't work on V3M as the
info structure is shared with V3H.

>  
>  	void __iomem *mmio;
>  	struct rcar_fcp_device *fcp;
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> index 159b68fa0829..206fe18c3c09 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> @@ -812,11 +812,57 @@ static const struct vsp1_device_info vsp1_device_infos[] = {
>  	},
>  };
>  
> +static const struct vsp1_device_info rzg2l_vsp2_device_info = {
> +	.version = VI6_IP_VERSION_MODEL_VSPD_RZG2L,
> +	.model = "VSP2-D",
> +	.gen = 3,
> +	.features = VSP1_HAS_BRS | VSP1_HAS_WPF_VFLIP | VSP1_HAS_EXT_DL,
> +	.lif_count = 1,
> +	.rpf_count = 2,
> +	.wpf_count = 1,
> +};
> +
> +static const struct vsp1_device_info *vsp1_lookup_info(struct vsp1_device *vsp1)
> +{
> +	const struct vsp1_device_info *info;
> +	unsigned int i;
> +
> +	/*
> +	 * Try the info stored in match data first for devices that don't have
> +	 * a version register.
> +	 */
> +	info = of_device_get_match_data(vsp1->dev);
> +	if (info) {
> +		vsp1->quirks = LIF_BUF_ATTR_QUIRK;
> +		vsp1->version = VI6_IP_VERSION_VSP_SW | info->version |
> +				VI6_IP_VERSION_SOC_RZG2L;
> +
> +		return info;
> +	}
> +
> +	vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> +
> +	for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
> +		info = &vsp1_device_infos[i];
> +
> +		if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) == info->version) {
> +			if ((vsp1->version & VI6_IP_VERSION_MASK) ==
> +			    (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M))
> +				vsp1->quirks = LIF_BUF_ATTR_QUIRK;
> +
> +			return info;
> +		}
> +	}
> +
> +	dev_err(vsp1->dev, "unsupported IP version 0x%08x\n", vsp1->version);
> +
> +	return NULL;

I'd rather handle the error first:

	for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
		info = &vsp1_device_infos[i];

		if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) == info->version)
			break;
	}

	if (i == ARRAY_SIZE(vsp1_device_infos) {
		dev_err(vsp1->dev, "unsupported IP version 0x%08x\n",
			vsp1->version);
		return NULL;
	}

	if ((vsp1->version & VI6_IP_VERSION_MASK) ==
	    (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M))
		vsp1->quirks = LIF_BUF_ATTR_QUIRK;

	return info;

> +}
> +
>  static int vsp1_probe(struct platform_device *pdev)
>  {
>  	struct vsp1_device *vsp1;
>  	struct device_node *fcp_node;
> -	unsigned int i;
>  	int ret;
>  	int irq;
>  
> @@ -872,26 +918,13 @@ static int vsp1_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto done;
>  
> -	vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> -
> -	for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
> -		if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) ==
> -		    vsp1_device_infos[i].version) {
> -			vsp1->info = &vsp1_device_infos[i];
> -			break;
> -		}
> -	}
> -
> +	vsp1->info = vsp1_lookup_info(vsp1);
>  	if (!vsp1->info) {
> -		dev_err(&pdev->dev, "unsupported IP version 0x%08x\n",
> -			vsp1->version);
>  		vsp1_device_put(vsp1);
>  		ret = -ENXIO;
>  		goto done;
>  	}
>  
> -	dev_dbg(&pdev->dev, "IP version 0x%08x\n", vsp1->version);

Any particular reason to drop this message ?

> -
>  	/*
>  	 * Previous use of the hardware (e.g. by the bootloader) could leave
>  	 * some interrupts enabled and pending.
> @@ -941,6 +974,7 @@ static int vsp1_remove(struct platform_device *pdev)
>  static const struct of_device_id vsp1_of_match[] = {
>  	{ .compatible = "renesas,vsp1" },
>  	{ .compatible = "renesas,vsp2" },
> +	{ .compatible = "renesas,rzg2l-vsp2", .data = &rzg2l_vsp2_device_info },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, vsp1_of_match);
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_lif.c b/drivers/media/platform/renesas/vsp1/vsp1_lif.c
> index 6a6857ac9327..e041871185d2 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_lif.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_lif.c
> @@ -107,6 +107,7 @@ static void lif_configure_stream(struct vsp1_entity *entity,
>  
>  	case VI6_IP_VERSION_MODEL_VSPDL_GEN3:
>  	case VI6_IP_VERSION_MODEL_VSPD_V3:
> +	case VI6_IP_VERSION_MODEL_VSPD_RZG2L:
>  		hbth = 0;
>  		obth = 1500;
>  		lbth = 0;
> @@ -130,13 +131,12 @@ static void lif_configure_stream(struct vsp1_entity *entity,
>  			VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN);
>  
>  	/*
> -	 * On R-Car V3M the LIF0 buffer attribute register has to be set to a
> -	 * non-default value to guarantee proper operation (otherwise artifacts
> -	 * may appear on the output). The value required by the manual is not
> -	 * explained but is likely a buffer size or threshold.
> +	 * On R-Car V3M and RZ/G2L the LIF0 buffer attribute register has to be
> +	 * set to a non-default value to guarantee proper operation (otherwise
> +	 * artifacts may appear on the output). The value required by the
> +	 * manual is not explained but is likely a buffer size or threshold.
>  	 */
> -	if ((entity->vsp1->version & VI6_IP_VERSION_MASK) ==
> -	    (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M))
> +	if (lif->quirks)

I don't think we need a new quirks field in the vsp1_lif structure, you
can access vsp1->quirks here.

>  		vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
>  			       VI6_LIF_LBA_LBA0 |
>  			       (1536 << VI6_LIF_LBA_LBA1_SHIFT));
> @@ -162,6 +162,7 @@ struct vsp1_lif *vsp1_lif_create(struct vsp1_device *vsp1, unsigned int index)
>  	lif->entity.ops = &lif_entity_ops;
>  	lif->entity.type = VSP1_ENTITY_LIF;
>  	lif->entity.index = index;
> +	lif->quirks = vsp1->quirks & LIF_BUF_ATTR_QUIRK;
>  
>  	/*
>  	 * The LIF is never exposed to userspace, but media entity registration
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_lif.h b/drivers/media/platform/renesas/vsp1/vsp1_lif.h
> index 71a4eda9c2b2..f073784e429b 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_lif.h
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_lif.h
> @@ -21,6 +21,7 @@ struct vsp1_device;
>  
>  struct vsp1_lif {
>  	struct vsp1_entity entity;
> +	u32 quirks;
>  };
>  
>  static inline struct vsp1_lif *to_lif(struct v4l2_subdev *subdev)
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_regs.h b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> index fae7286eb01e..41bea15597c9 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> @@ -767,6 +767,8 @@
>  #define VI6_IP_VERSION_MODEL_VSPDL_GEN3	(0x19 << 8)
>  #define VI6_IP_VERSION_MODEL_VSPBS_GEN3	(0x1a << 8)
>  #define VI6_IP_VERSION_MODEL_VSPD_V3U	(0x1c << 8)
> +/* RZ/G2L SoC's have no version register, So use 0x80 as the model version */
> +#define VI6_IP_VERSION_MODEL_VSPD_RZG2L	(0x80 << 8)
>  
>  #define VI6_IP_VERSION_SOC_MASK		(0xff << 0)
>  #define VI6_IP_VERSION_SOC_H2		(0x01 << 0)
> @@ -780,6 +782,11 @@
>  #define VI6_IP_VERSION_SOC_M3N		(0x04 << 0)
>  #define VI6_IP_VERSION_SOC_E3		(0x04 << 0)
>  #define VI6_IP_VERSION_SOC_V3U		(0x05 << 0)
> +/* RZ/G2L SoC's have no version register, So use 0x80 for SoC Identification */
> +#define VI6_IP_VERSION_SOC_RZG2L	(0x80 << 0)
> +
> +#define VI6_IP_VERSION_VSP_MASK		(0xffff << 16)
> +#define VI6_IP_VERSION_VSP_SW		(0xfffe << 16) /* SW VSP version */
>  
>  /* -----------------------------------------------------------------------------
>   * RPF CLUT Registers

-- 
Regards,

Laurent Pinchart

RE: [PATCH v7 1/3] media: dt-bindings: media: renesas,vsp1: Document RZ/{G2L,V2L} VSPD bindings To: Mauro Carvalho Chehab <mchehab@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>

[Biju Das]

Hi Laurent,

Thanks for the feedback.

> Subject: Re: [PATCH v7 1/3] media: dt-bindings: media: renesas,vsp1:
> Document RZ/{G2L,V2L} VSPD bindings 
> 
> Hi Biju,
> 
> Thank you for the patch.
> 
> On Thu, Apr 14, 2022 at 03:26:03PM +0100, Biju Das wrote:
> > Document VSPD found in RZ/G2L and RZ/V2L family SoC's. VSPD block is
> > similar to VSP2-D found on R-Car SoC's, but it does not have a version
> > register and it has 3 clocks compared to 1 clock on vsp1 and vsp2.
> >
> > This patch introduces a new compatible 'renesas,rzg2l-vsp2' to handle
> > these differences.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> > ---
> > v6->v7:
> >  * No change
> > v5->v6:
> >  * Removed LCDC reference clock description
> >  * Changed the clock name from du.0->aclk
> > v4->v5:
> >  * No change
> > v3->v4:
> >  * No change
> > v2->v3:
> >  * Added Rb tag from Krzysztof.
> > v1->v2:
> >  * Changed compatible from vsp2-rzg2l->rzg2l-vsp2
> > RFC->v1:
> >  * Updated commit description
> >  * Changed compatible from vsp2-r9a07g044->vsp2-rzg2l
> >  * Defined the clocks
> >  * Clock max Items is based on SoC Compatible string
> > RFC:
> >  *
> 
> > ---
> >  .../bindings/media/renesas,vsp1.yaml          | 52 ++++++++++++++-----
> >  1 file changed, 39 insertions(+), 13 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > index 990e9c1dbc43..a236b266fa4b 100644
> > --- a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > +++ b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > @@ -19,6 +19,7 @@ properties:
> >      enum:
> >        - renesas,vsp1 # R-Car Gen2 and RZ/G1
> >        - renesas,vsp2 # R-Car Gen3 and RZ/G2
> > +      - renesas,rzg2l-vsp2 # RZ/G2L and RZ/V2L
> >
> >    reg:
> >      maxItems: 1
> > @@ -26,8 +27,8 @@ properties:
> >    interrupts:
> >      maxItems: 1
> >
> > -  clocks:
> > -    maxItems: 1
> > +  clocks: true
> > +  clock-names: true
> 
> clock-names shouldn't be true here, as it should only be set on rzg2l-vsp2.
> I think you can actually drop both clocks and clock-names here.

If I drop clocks, then I get below dt_binding_check error

biju@biju-VirtualBox:~/rzg2l-linux$ make ARCH=arm64 DT_CHECKER_FLAGS=-m DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/renesas,vsp1.yaml CROSS_COMPILE=~/gcc-arm-10.3-2021.07-x86_64-aarch64-none-linux-gnu/bin/aarch64-none-linux-gnu- dt_binding_check -j8
  LINT    Documentation/devicetree/bindings
  DTEX    Documentation/devicetree/bindings/media/renesas,vsp1.example.dts
  CHKDT   Documentation/devicetree/bindings/processed-schema.json
  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
  DTC     Documentation/devicetree/bindings/media/renesas,vsp1.example.dtb
  CHECK   Documentation/devicetree/bindings/media/renesas,vsp1.example.dtb
/home/biju/rzg2l-linux/Documentation/devicetree/bindings/media/renesas,vsp1.example.dtb: vsp@fe928000: 'clocks' does not match any of the regexes: 'pinctrl-[0-9]+'
	From schema: /home/biju/rzg2l-linux/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
/home/biju/rzg2l-linux/Documentation/devicetree/bindings/media/renesas,vsp1.example.dtb: vsp@fe920000: 'clocks' does not match any of the regexes: 'pinctrl-[0-9]+'
	From schema: /home/biju/rzg2l-linux/Documentation/devicetree/bindings/media/renesas,vsp1.yaml

If I drop clock-names, I get dtbs-check error for RZ/G2{L,LC},

make ARCH=arm64 DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/renesas,vsp1.yaml CROSS_COMPILE=~/gcc-arm-10.3-2021.07-x86_64-aarch64-none-linux-gnu/bin/aarch64-none-linux-gnu- dtbs_check -j8

/home/biju/rzg2l-linux/arch/arm64/boot/dts/renesas/r9a07g044c2-smarc.dtb: vsp@10870000: 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+'
	From schema: /home/biju/rzg2l-linux/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
/home/biju/rzg2l-linux/arch/arm64/boot/dts/renesas/r9a07g044l2-smarc.dtb: vsp@10870000: 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+'
	From schema: /home/biju/rzg2l-linux/Documentation/devicetree/bindings/media/renesas,vsp1.yaml

So looks like both are required.

Please correct me, If I am missing anything here.

Cheers,
Biju

> 
> With this addressed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >
> >    power-domains:
> >      maxItems: 1
> > @@ -50,17 +51,42 @@ required:
> >
> >  additionalProperties: false
> >
> > -if:
> > -  properties:
> > -    compatible:
> > -      items:
> > -        - const: renesas,vsp1
> > -then:
> > -  properties:
> > -    renesas,fcp: false
> > -else:
> > -  required:
> > -    - renesas,fcp
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: renesas,vsp1
> > +    then:
> > +      properties:
> > +        renesas,fcp: false
> > +    else:
> > +      required:
> > +        - renesas,fcp
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: renesas,rzg2l-vsp2
> > +    then:
> > +      properties:
> > +        clocks:
> > +          items:
> > +            - description: Main clock
> > +            - description: Register access clock
> > +            - description: Video clock
> > +        clock-names:
> > +          items:
> > +            - const: aclk
> > +            - const: pclk
> > +            - const: vclk
> > +      required:
> > +        - clock-names
> > +    else:
> > +      properties:
> > +        clocks:
> > +          maxItems: 1
> >
> >  examples:
> >    # R8A7790 (R-Car H2) VSP1-S
> 
> --
> Regards,
> 
> Laurent Pinchart

RE: [PATCH v7 3/3] media: renesas: vsp1: Add support for RZ/G2L VSPD

[Biju Das]

Hi Laurent,

Thanks for the feedback

> Subject: Re: [PATCH v7 3/3] media: renesas: vsp1: Add support for RZ/G2L
> VSPD
> 
> Hi Biju,
> 
> Thank you for the patch.
> 
> On Thu, Apr 14, 2022 at 03:26:05PM +0100, Biju Das wrote:
> > The RZ/G2L VSPD provides a single VSPD instance. It has the following
> > sub modules MAU, CTU, RPF, DPR, LUT, BRS, WPF and LIF.
> >
> > The VSPD block on RZ/G2L does not have a version register, so added a
> > new compatible string "renesas,rzg2l-vsp2" with a data pointer
> > containing the info structure. Also the reset line is shared with the DU
> module.
> >
> > Apart from this, added a quirk to handle LIF0 buffer attribute to fix
> > the artifacts on the output for both R-Car V3M and RZ/G2L alike SoC's.
> 
> That's quite a few changes, it should have been split in multiple patches
> (one that adds the software version mechanism, one that extends the LIF
> buffer attribute handling mechanism, and one that adds RZ/G2L support). You
> don't have to split the changes in the next version, but please keep it in
> mind for future series.

OK, will take care this in future series.

> 
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > ---
> > v6->v7:
> >  * Added Rb tag from Kieran
> >  * Added a quirk to handle LIF0 buffer attribute related
> >    changes for V3M and G2L.
> >  * Removed the macro for VSP HW version
> > v5->v6:
> >  * Rebased to media_staging and updated commit header
> >  * Removed the extra tab from rzg2l_vsp2_device_info
> >  * Changed the function vsp1_lookup->vsp1_lookup_info and
> >    all info match related code moved here.
> >  * Add VI6_IP_VERSION_VSP and VI6_IP_VERSION_VSP_SW macros to
> >    distinguish HW & SW IP_VSP_Version.
> >  * Used 0x80 for RZG2L VSPD model and SoC identification
> >  * Updated Switch() for LIF0 buffer attribute handling.
> > v4->v5:
> >  * Fixed typo VI6_IP_VERSION_MODEL_MASK->VI6_IP_VERSION_MASK
> >  * To be consistent with other SoC's, introduced VI6_IP_VERSION_SOC_G2L
> >    for RZ/G2L SoC's.
> > v3->v4:
> >  * Added Rb tag from Geert
> >  * Add switch() for LIF0 buffer attribute handling for RZ/G2L and V3M
> > v2->v3:
> >  * Fixed version comparison in vsp1_lookup()
> > v1->v2:
> >  * Changed the compatible from vsp2-rzg2l->rzg2l-vsp2
> >  * Added standalone device info for rzg2l-vsp2.
> >  * Added vsp1_lookup helper function.
> >  * Updated comments for LIF0 buffer attribute register
> >  * Used last ID for rzg2l-vsp2.
> > RFC->v1:
> >  * Used data pointer containing info structure to retrieve version
> > information
> > RFC:
> 
> > ---
> >  drivers/media/platform/renesas/vsp1/vsp1.h    |  3 +
> >  .../media/platform/renesas/vsp1/vsp1_drv.c    | 64 ++++++++++++++-----
> >  .../media/platform/renesas/vsp1/vsp1_lif.c    | 13 ++--
> >  .../media/platform/renesas/vsp1/vsp1_lif.h    |  1 +
> >  .../media/platform/renesas/vsp1/vsp1_regs.h   |  7 ++
> >  5 files changed, 67 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1.h
> > b/drivers/media/platform/renesas/vsp1/vsp1.h
> > index baf898d577ec..de3685cc89f3 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1.h
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1.h
> > @@ -56,6 +56,8 @@ struct vsp1_uif;
> >  #define VSP1_HAS_BRS		BIT(9)
> >  #define VSP1_HAS_EXT_DL		BIT(10)
> >
> > +#define LIF_BUF_ATTR_QUIRK	BIT(0)
> > +
> >  struct vsp1_device_info {
> >  	u32 version;
> >  	const char *model;
> > @@ -76,6 +78,7 @@ struct vsp1_device {
> >  	struct device *dev;
> >  	const struct vsp1_device_info *info;
> >  	u32 version;
> > +	u32 quirks;
> 
> It would have been nice to make this a feature bit in the vsp1_device_info
> structure instead, but that won't work on V3M as the info structure is
> shared with V3H.

Yep, I agree. 

> 
> >
> >  	void __iomem *mmio;
> >  	struct rcar_fcp_device *fcp;
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > index 159b68fa0829..206fe18c3c09 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > @@ -812,11 +812,57 @@ static const struct vsp1_device_info
> vsp1_device_infos[] = {
> >  	},
> >  };
> >
> > +static const struct vsp1_device_info rzg2l_vsp2_device_info = {
> > +	.version = VI6_IP_VERSION_MODEL_VSPD_RZG2L,
> > +	.model = "VSP2-D",
> > +	.gen = 3,
> > +	.features = VSP1_HAS_BRS | VSP1_HAS_WPF_VFLIP | VSP1_HAS_EXT_DL,
> > +	.lif_count = 1,
> > +	.rpf_count = 2,
> > +	.wpf_count = 1,
> > +};
> > +
> > +static const struct vsp1_device_info *vsp1_lookup_info(struct
> > +vsp1_device *vsp1) {
> > +	const struct vsp1_device_info *info;
> > +	unsigned int i;
> > +
> > +	/*
> > +	 * Try the info stored in match data first for devices that don't
> have
> > +	 * a version register.
> > +	 */
> > +	info = of_device_get_match_data(vsp1->dev);
> > +	if (info) {
> > +		vsp1->quirks = LIF_BUF_ATTR_QUIRK;
> > +		vsp1->version = VI6_IP_VERSION_VSP_SW | info->version |
> > +				VI6_IP_VERSION_SOC_RZG2L;
> > +
> > +		return info;
> > +	}
> > +
> > +	vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
> > +		info = &vsp1_device_infos[i];
> > +
> > +		if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) == info-
> >version) {
> > +			if ((vsp1->version & VI6_IP_VERSION_MASK) ==
> > +			    (VI6_IP_VERSION_MODEL_VSPD_V3 |
> VI6_IP_VERSION_SOC_V3M))
> > +				vsp1->quirks = LIF_BUF_ATTR_QUIRK;
> > +
> > +			return info;
> > +		}
> > +	}
> > +
> > +	dev_err(vsp1->dev, "unsupported IP version 0x%08x\n",
> > +vsp1->version);
> > +
> > +	return NULL;
> 
> I'd rather handle the error first:

Agreed.

> 
> 	for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
> 		info = &vsp1_device_infos[i];
> 
> 		if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) == info-
> >version)
> 			break;
> 	}
> 
> 	if (i == ARRAY_SIZE(vsp1_device_infos) {
> 		dev_err(vsp1->dev, "unsupported IP version 0x%08x\n",
> 			vsp1->version);
> 		return NULL;
> 	}
> 
> 	if ((vsp1->version & VI6_IP_VERSION_MASK) ==
> 	    (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M))
> 		vsp1->quirks = LIF_BUF_ATTR_QUIRK;
> 
> 	return info;
> 
> > +}
> > +
> >  static int vsp1_probe(struct platform_device *pdev)  {
> >  	struct vsp1_device *vsp1;
> >  	struct device_node *fcp_node;
> > -	unsigned int i;
> >  	int ret;
> >  	int irq;
> >
> > @@ -872,26 +918,13 @@ static int vsp1_probe(struct platform_device *pdev)
> >  	if (ret < 0)
> >  		goto done;
> >
> > -	vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> > -
> > -	for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
> > -		if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) ==
> > -		    vsp1_device_infos[i].version) {
> > -			vsp1->info = &vsp1_device_infos[i];
> > -			break;
> > -		}
> > -	}
> > -
> > +	vsp1->info = vsp1_lookup_info(vsp1);
> >  	if (!vsp1->info) {
> > -		dev_err(&pdev->dev, "unsupported IP version 0x%08x\n",
> > -			vsp1->version);
> >  		vsp1_device_put(vsp1);
> >  		ret = -ENXIO;
> >  		goto done;
> >  	}
> >
> > -	dev_dbg(&pdev->dev, "IP version 0x%08x\n", vsp1->version);
> 
> Any particular reason to drop this message ?

It is deleted by mistake.

> 
> > -
> >  	/*
> >  	 * Previous use of the hardware (e.g. by the bootloader) could leave
> >  	 * some interrupts enabled and pending.
> > @@ -941,6 +974,7 @@ static int vsp1_remove(struct platform_device
> > *pdev)  static const struct of_device_id vsp1_of_match[] = {
> >  	{ .compatible = "renesas,vsp1" },
> >  	{ .compatible = "renesas,vsp2" },
> > +	{ .compatible = "renesas,rzg2l-vsp2", .data =
> > +&rzg2l_vsp2_device_info },
> >  	{ },
> >  };
> >  MODULE_DEVICE_TABLE(of, vsp1_of_match); diff --git
> > a/drivers/media/platform/renesas/vsp1/vsp1_lif.c
> > b/drivers/media/platform/renesas/vsp1/vsp1_lif.c
> > index 6a6857ac9327..e041871185d2 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_lif.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_lif.c
> > @@ -107,6 +107,7 @@ static void lif_configure_stream(struct
> > vsp1_entity *entity,
> >
> >  	case VI6_IP_VERSION_MODEL_VSPDL_GEN3:
> >  	case VI6_IP_VERSION_MODEL_VSPD_V3:
> > +	case VI6_IP_VERSION_MODEL_VSPD_RZG2L:
> >  		hbth = 0;
> >  		obth = 1500;
> >  		lbth = 0;
> > @@ -130,13 +131,12 @@ static void lif_configure_stream(struct vsp1_entity
> *entity,
> >  			VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN);
> >
> >  	/*
> > -	 * On R-Car V3M the LIF0 buffer attribute register has to be set to
> a
> > -	 * non-default value to guarantee proper operation (otherwise
> artifacts
> > -	 * may appear on the output). The value required by the manual is
> not
> > -	 * explained but is likely a buffer size or threshold.
> > +	 * On R-Car V3M and RZ/G2L the LIF0 buffer attribute register has to
> be
> > +	 * set to a non-default value to guarantee proper operation
> (otherwise
> > +	 * artifacts may appear on the output). The value required by the
> > +	 * manual is not explained but is likely a buffer size or threshold.
> >  	 */
> > -	if ((entity->vsp1->version & VI6_IP_VERSION_MASK) ==
> > -	    (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M))
> > +	if (lif->quirks)
> 
> I don't think we need a new quirks field in the vsp1_lif structure, you can
> access vsp1->quirks here.

Good point, I missed it.

Cheers,
Biju


> 
> >  		vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
> >  			       VI6_LIF_LBA_LBA0 |
> >  			       (1536 << VI6_LIF_LBA_LBA1_SHIFT)); @@ -162,6
> +162,7 @@
> > struct vsp1_lif *vsp1_lif_create(struct vsp1_device *vsp1, unsigned int
> index)
> >  	lif->entity.ops = &lif_entity_ops;
> >  	lif->entity.type = VSP1_ENTITY_LIF;
> >  	lif->entity.index = index;
> > +	lif->quirks = vsp1->quirks & LIF_BUF_ATTR_QUIRK;
> >
> >  	/*
> >  	 * The LIF is never exposed to userspace, but media entity
> > registration diff --git
> > a/drivers/media/platform/renesas/vsp1/vsp1_lif.h
> > b/drivers/media/platform/renesas/vsp1/vsp1_lif.h
> > index 71a4eda9c2b2..f073784e429b 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_lif.h
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_lif.h
> > @@ -21,6 +21,7 @@ struct vsp1_device;
> >
> >  struct vsp1_lif {
> >  	struct vsp1_entity entity;
> > +	u32 quirks;
> >  };
> >
> >  static inline struct vsp1_lif *to_lif(struct v4l2_subdev *subdev)
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> > b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> > index fae7286eb01e..41bea15597c9 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> > @@ -767,6 +767,8 @@
> >  #define VI6_IP_VERSION_MODEL_VSPDL_GEN3	(0x19 << 8)
> >  #define VI6_IP_VERSION_MODEL_VSPBS_GEN3	(0x1a << 8)
> >  #define VI6_IP_VERSION_MODEL_VSPD_V3U	(0x1c << 8)
> > +/* RZ/G2L SoC's have no version register, So use 0x80 as the model
> version */
> > +#define VI6_IP_VERSION_MODEL_VSPD_RZG2L	(0x80 << 8)
> >
> >  #define VI6_IP_VERSION_SOC_MASK		(0xff << 0)
> >  #define VI6_IP_VERSION_SOC_H2		(0x01 << 0)
> > @@ -780,6 +782,11 @@
> >  #define VI6_IP_VERSION_SOC_M3N		(0x04 << 0)
> >  #define VI6_IP_VERSION_SOC_E3		(0x04 << 0)
> >  #define VI6_IP_VERSION_SOC_V3U		(0x05 << 0)
> > +/* RZ/G2L SoC's have no version register, So use 0x80 for SoC
> Identification */
> > +#define VI6_IP_VERSION_SOC_RZG2L	(0x80 << 0)
> > +
> > +#define VI6_IP_VERSION_VSP_MASK		(0xffff << 16)
> > +#define VI6_IP_VERSION_VSP_SW		(0xfffe << 16) /* SW VSP version
> */
> >
> >  /* ---------------------------------------------------------------------
> --------
> >   * RPF CLUT Registers
> 
> --
> Regards,
> 
> Laurent Pinchart

Re: [PATCH v7 1/3] media: dt-bindings: media: renesas,vsp1: Document RZ/{G2L,V2L} VSPD bindings To: Mauro Carvalho Chehab <mchehab@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>

[Laurent Pinchart]

Hi Biju,

On Mon, Apr 18, 2022 at 07:34:19PM +0000, Biju Das wrote:
> > Subject: Re: [PATCH v7 1/3] media: dt-bindings: media: renesas,vsp1:
> > Document RZ/{G2L,V2L} VSPD bindings 
> > 
> > Hi Biju,
> > 
> > Thank you for the patch.
> > 
> > On Thu, Apr 14, 2022 at 03:26:03PM +0100, Biju Das wrote:
> > > Document VSPD found in RZ/G2L and RZ/V2L family SoC's. VSPD block is
> > > similar to VSP2-D found on R-Car SoC's, but it does not have a version
> > > register and it has 3 clocks compared to 1 clock on vsp1 and vsp2.
> > >
> > > This patch introduces a new compatible 'renesas,rzg2l-vsp2' to handle
> > > these differences.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> > > ---
> > > v6->v7:
> > >  * No change
> > > v5->v6:
> > >  * Removed LCDC reference clock description
> > >  * Changed the clock name from du.0->aclk
> > > v4->v5:
> > >  * No change
> > > v3->v4:
> > >  * No change
> > > v2->v3:
> > >  * Added Rb tag from Krzysztof.
> > > v1->v2:
> > >  * Changed compatible from vsp2-rzg2l->rzg2l-vsp2
> > > RFC->v1:
> > >  * Updated commit description
> > >  * Changed compatible from vsp2-r9a07g044->vsp2-rzg2l
> > >  * Defined the clocks
> > >  * Clock max Items is based on SoC Compatible string
> > > RFC:
> > >  *
> > 
> > > ---
> > >  .../bindings/media/renesas,vsp1.yaml          | 52 ++++++++++++++-----
> > >  1 file changed, 39 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > > b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > > index 990e9c1dbc43..a236b266fa4b 100644
> > > --- a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > > +++ b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > > @@ -19,6 +19,7 @@ properties:
> > >      enum:
> > >        - renesas,vsp1 # R-Car Gen2 and RZ/G1
> > >        - renesas,vsp2 # R-Car Gen3 and RZ/G2
> > > +      - renesas,rzg2l-vsp2 # RZ/G2L and RZ/V2L
> > >
> > >    reg:
> > >      maxItems: 1
> > > @@ -26,8 +27,8 @@ properties:
> > >    interrupts:
> > >      maxItems: 1
> > >
> > > -  clocks:
> > > -    maxItems: 1
> > > +  clocks: true
> > > +  clock-names: true
> > 
> > clock-names shouldn't be true here, as it should only be set on rzg2l-vsp2.
> > I think you can actually drop both clocks and clock-names here.
> 
> If I drop clocks, then I get below dt_binding_check error
> 
> biju@biju-VirtualBox:~/rzg2l-linux$ make ARCH=arm64 DT_CHECKER_FLAGS=-m DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/renesas,vsp1.yaml CROSS_COMPILE=~/gcc-arm-10.3-2021.07-x86_64-aarch64-none-linux-gnu/bin/aarch64-none-linux-gnu- dt_binding_check -j8
>   LINT    Documentation/devicetree/bindings
>   DTEX    Documentation/devicetree/bindings/media/renesas,vsp1.example.dts
>   CHKDT   Documentation/devicetree/bindings/processed-schema.json
>   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
>   DTC     Documentation/devicetree/bindings/media/renesas,vsp1.example.dtb
>   CHECK   Documentation/devicetree/bindings/media/renesas,vsp1.example.dtb
> /home/biju/rzg2l-linux/Documentation/devicetree/bindings/media/renesas,vsp1.example.dtb: vsp@fe928000: 'clocks' does not match any of the regexes: 'pinctrl-[0-9]+'
> 	From schema: /home/biju/rzg2l-linux/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> /home/biju/rzg2l-linux/Documentation/devicetree/bindings/media/renesas,vsp1.example.dtb: vsp@fe920000: 'clocks' does not match any of the regexes: 'pinctrl-[0-9]+'
> 	From schema: /home/biju/rzg2l-linux/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> 
> If I drop clock-names, I get dtbs-check error for RZ/G2{L,LC},
> 
> make ARCH=arm64 DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/renesas,vsp1.yaml CROSS_COMPILE=~/gcc-arm-10.3-2021.07-x86_64-aarch64-none-linux-gnu/bin/aarch64-none-linux-gnu- dtbs_check -j8
> 
> /home/biju/rzg2l-linux/arch/arm64/boot/dts/renesas/r9a07g044c2-smarc.dtb: vsp@10870000: 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+'
> 	From schema: /home/biju/rzg2l-linux/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> /home/biju/rzg2l-linux/arch/arm64/boot/dts/renesas/r9a07g044l2-smarc.dtb: vsp@10870000: 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+'
> 	From schema: /home/biju/rzg2l-linux/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> 
> So looks like both are required.

Indeed, we would need to switch from additionalProperties to
unevaluatedProperties then, and that's not allowed for schemas without a
$ref.

> Please correct me, If I am missing anything here.

Let's keep both properties here, but then ... (see below)

> > With this addressed,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > >
> > >    power-domains:
> > >      maxItems: 1
> > > @@ -50,17 +51,42 @@ required:
> > >
> > >  additionalProperties: false
> > >
> > > -if:
> > > -  properties:
> > > -    compatible:
> > > -      items:
> > > -        - const: renesas,vsp1
> > > -then:
> > > -  properties:
> > > -    renesas,fcp: false
> > > -else:
> > > -  required:
> > > -    - renesas,fcp
> > > +allOf:
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: renesas,vsp1
> > > +    then:
> > > +      properties:
> > > +        renesas,fcp: false
> > > +    else:
> > > +      required:
> > > +        - renesas,fcp
> > > +
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: renesas,rzg2l-vsp2
> > > +    then:
> > > +      properties:
> > > +        clocks:
> > > +          items:
> > > +            - description: Main clock
> > > +            - description: Register access clock
> > > +            - description: Video clock
> > > +        clock-names:
> > > +          items:
> > > +            - const: aclk
> > > +            - const: pclk
> > > +            - const: vclk
> > > +      required:
> > > +        - clock-names
> > > +    else:
> > > +      properties:
> > > +        clocks:
> > > +          maxItems: 1

... you will need

        clock-names: false

here.

> > >
> > >  examples:
> > >    # R8A7790 (R-Car H2) VSP1-S

-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 1/3] media: dt-bindings: media: renesas,vsp1: Document RZ/{G2L,V2L} VSPD bindings To: Mauro Carvalho Chehab <mchehab@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>

[Rob Herring]

On Tue, Apr 19, 2022 at 11:29:39AM +0300, Laurent Pinchart wrote:
> Hi Biju,
> 
> On Mon, Apr 18, 2022 at 07:34:19PM +0000, Biju Das wrote:
> > > Subject: Re: [PATCH v7 1/3] media: dt-bindings: media: renesas,vsp1:
> > > Document RZ/{G2L,V2L} VSPD bindings 
> > > 
> > > Hi Biju,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Thu, Apr 14, 2022 at 03:26:03PM +0100, Biju Das wrote:
> > > > Document VSPD found in RZ/G2L and RZ/V2L family SoC's. VSPD block is
> > > > similar to VSP2-D found on R-Car SoC's, but it does not have a version
> > > > register and it has 3 clocks compared to 1 clock on vsp1 and vsp2.
> > > >
> > > > This patch introduces a new compatible 'renesas,rzg2l-vsp2' to handle
> > > > these differences.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> > > > ---
> > > > v6->v7:
> > > >  * No change
> > > > v5->v6:
> > > >  * Removed LCDC reference clock description
> > > >  * Changed the clock name from du.0->aclk
> > > > v4->v5:
> > > >  * No change
> > > > v3->v4:
> > > >  * No change
> > > > v2->v3:
> > > >  * Added Rb tag from Krzysztof.
> > > > v1->v2:
> > > >  * Changed compatible from vsp2-rzg2l->rzg2l-vsp2
> > > > RFC->v1:
> > > >  * Updated commit description
> > > >  * Changed compatible from vsp2-r9a07g044->vsp2-rzg2l
> > > >  * Defined the clocks
> > > >  * Clock max Items is based on SoC Compatible string
> > > > RFC:
> > > >  *
> > > 
> > > > ---
> > > >  .../bindings/media/renesas,vsp1.yaml          | 52 ++++++++++++++-----
> > > >  1 file changed, 39 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > > > b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > > > index 990e9c1dbc43..a236b266fa4b 100644
> > > > --- a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > > > +++ b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > > > @@ -19,6 +19,7 @@ properties:
> > > >      enum:
> > > >        - renesas,vsp1 # R-Car Gen2 and RZ/G1
> > > >        - renesas,vsp2 # R-Car Gen3 and RZ/G2
> > > > +      - renesas,rzg2l-vsp2 # RZ/G2L and RZ/V2L
> > > >
> > > >    reg:
> > > >      maxItems: 1
> > > > @@ -26,8 +27,8 @@ properties:
> > > >    interrupts:
> > > >      maxItems: 1
> > > >
> > > > -  clocks:
> > > > -    maxItems: 1
> > > > +  clocks: true
> > > > +  clock-names: true
> > > 
> > > clock-names shouldn't be true here, as it should only be set on rzg2l-vsp2.
> > > I think you can actually drop both clocks and clock-names here.
> > 
> > If I drop clocks, then I get below dt_binding_check error
> > 
> > biju@biju-VirtualBox:~/rzg2l-linux$ make ARCH=arm64 DT_CHECKER_FLAGS=-m DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/renesas,vsp1.yaml CROSS_COMPILE=~/gcc-arm-10.3-2021.07-x86_64-aarch64-none-linux-gnu/bin/aarch64-none-linux-gnu- dt_binding_check -j8
> >   LINT    Documentation/devicetree/bindings
> >   DTEX    Documentation/devicetree/bindings/media/renesas,vsp1.example.dts
> >   CHKDT   Documentation/devicetree/bindings/processed-schema.json
> >   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
> >   DTC     Documentation/devicetree/bindings/media/renesas,vsp1.example.dtb
> >   CHECK   Documentation/devicetree/bindings/media/renesas,vsp1.example.dtb
> > /home/biju/rzg2l-linux/Documentation/devicetree/bindings/media/renesas,vsp1.example.dtb: vsp@fe928000: 'clocks' does not match any of the regexes: 'pinctrl-[0-9]+'
> > 	From schema: /home/biju/rzg2l-linux/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > /home/biju/rzg2l-linux/Documentation/devicetree/bindings/media/renesas,vsp1.example.dtb: vsp@fe920000: 'clocks' does not match any of the regexes: 'pinctrl-[0-9]+'
> > 	From schema: /home/biju/rzg2l-linux/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > 
> > If I drop clock-names, I get dtbs-check error for RZ/G2{L,LC},
> > 
> > make ARCH=arm64 DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/renesas,vsp1.yaml CROSS_COMPILE=~/gcc-arm-10.3-2021.07-x86_64-aarch64-none-linux-gnu/bin/aarch64-none-linux-gnu- dtbs_check -j8
> > 
> > /home/biju/rzg2l-linux/arch/arm64/boot/dts/renesas/r9a07g044c2-smarc.dtb: vsp@10870000: 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+'
> > 	From schema: /home/biju/rzg2l-linux/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > /home/biju/rzg2l-linux/arch/arm64/boot/dts/renesas/r9a07g044l2-smarc.dtb: vsp@10870000: 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+'
> > 	From schema: /home/biju/rzg2l-linux/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > 
> > So looks like both are required.
> 
> Indeed, we would need to switch from additionalProperties to
> unevaluatedProperties then, and that's not allowed for schemas without a
> $ref.

The issue is here if we allow properties to be defined in if/then 
schemas, we can't have some meta-schema checks on them as we don't know 
if the property is defined elsewhere or not. That's primarily a problem 
for vendor specific properties where we need to ensure a type and 
description.

Rob

Re: [PATCH v7 1/3] media: dt-bindings: media: renesas,vsp1: Document RZ/{G2L,V2L} VSPD bindings To: Mauro Carvalho Chehab <mchehab@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>

[Geert Uytterhoeven]

Hi Biju,

On Thu, Apr 14, 2022 at 4:26 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Document VSPD found in RZ/G2L and RZ/V2L family SoC's. VSPD block is
> similar to VSP2-D found on R-Car SoC's, but it does not have a version
> register and it has 3 clocks compared to 1 clock on vsp1 and vsp2.
>
> This patch introduces a new compatible 'renesas,rzg2l-vsp2' to handle
> these differences.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

Thanks for your patch!

> --- a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> +++ b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> @@ -19,6 +19,7 @@ properties:
>      enum:
>        - renesas,vsp1 # R-Car Gen2 and RZ/G1
>        - renesas,vsp2 # R-Car Gen3 and RZ/G2
> +      - renesas,rzg2l-vsp2 # RZ/G2L and RZ/V2L

Given there is no version register, probably you want to define
SoC-specific compatible values.

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 v7 1/3] media: dt-bindings: media: renesas,vsp1: Document RZ/{G2L,V2L} VSPD bindings To: Mauro Carvalho Chehab <mchehab@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v7 1/3] media: dt-bindings: media: renesas,vsp1:
> Document RZ/{G2L,V2L} VSPD bindings To: Mauro Carvalho Chehab
> <mchehab@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski
> <krzk+dt@kernel.org>
> 
> Hi Biju,
> 
> On Thu, Apr 14, 2022 at 4:26 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > Document VSPD found in RZ/G2L and RZ/V2L family SoC's. VSPD block is
> > similar to VSP2-D found on R-Car SoC's, but it does not have a version
> > register and it has 3 clocks compared to 1 clock on vsp1 and vsp2.
> >
> > This patch introduces a new compatible 'renesas,rzg2l-vsp2' to handle
> > these differences.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> 
> Thanks for your patch!
> 
> > --- a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > +++ b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > @@ -19,6 +19,7 @@ properties:
> >      enum:
> >        - renesas,vsp1 # R-Car Gen2 and RZ/G1
> >        - renesas,vsp2 # R-Car Gen3 and RZ/G2
> > +      - renesas,rzg2l-vsp2 # RZ/G2L and RZ/V2L
> 
> Given there is no version register, probably you want to define SoC-
> specific compatible values.

Yes, that is the plan which is in line with Kieran's suggestion.

Cheers,
Biju