[PATCH v11 0/5] Add support for RZ/G2L VSPD

[PATCH v11 0/5] 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,r9a07g044-vsp2" with a data pointer containing
the info structure. Also the reset line is shared with the DU module.

This patch series is tested on RZ/G1N, RZ/G2M and RZ/G2L boards.

v10->v11:
 * Added poll for reset status in order to avoid lock-up on R-Car Gen2
 * with vsp register access after deassert.

v9->v10
 * Moved {deassert,assert} calls to vsp1_pm_runtime_{resume,suspend}

V8->v9
 * Added Rb tag from Geert for patch#3
 * Replaced break with return info in case a Model match is found and
   removed additional check for non-match case.
 * Used generic check for matching SoCs with LBA feature.
 * Replaced the code comments RZ/G2L {SoC's,SoC} with RZ/G2L SoCs.
v7->v8:
 * Split the patch for adding s/w version, feature bit and RZ/G2L support
 * Added feature bit VSP1_HAS_NON_ZERO_LBA to device_info
 * Added .soc for RZ/G2L
 * Replaced the compatible "renesas,rzg2l-vsp2" -> "renesas,r9a07g044-vsp2"
 * Updated Clock-names to false for non RZ/G2L SoC's on binding doc
 * Added Rb tag from Laurent for bindings
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 (5):
  media: dt-bindings: media: renesas,vsp1: Document RZ/G2L VSPD bindings
  media: renesas: vsp1: Add support to deassert/assert reset line
  media: renesas: vsp1: Add support for VSP software version
  media: renesas: vsp1: Add VSP1_HAS_NON_ZERO_LBA feature bit
  media: renesas: vsp1: Add support for RZ/G2L VSPD

 .../bindings/media/renesas,vsp1.yaml          |  53 ++++++---
 drivers/media/platform/renesas/vsp1/vsp1.h    |   4 +
 .../media/platform/renesas/vsp1/vsp1_drv.c    | 104 +++++++++++++++---
 .../media/platform/renesas/vsp1/vsp1_lif.c    |  12 +-
 .../media/platform/renesas/vsp1/vsp1_regs.h   |   6 +
 5 files changed, 145 insertions(+), 34 deletions(-)

-- 
2.25.1

[PATCH v11 1/5] media: dt-bindings: media: renesas,vsp1: Document RZ/G2L VSPD bindings

[Biju Das]

Document VSPD found in RZ/G2L SoC. 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,r9a07g044-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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
v10->v11:
 * No change
v9->v10:
 * No change
v8->v9:
 * No change
v7->v8:
 * Added Clock-names to false for Non RZ/G2L SoC's
 * Replaced compatble 'renesas,rzg2l-vsp2'->'renesas,r9a07g044-vsp2'
 * Removed RZ/V2L SoC, will be added later after testing it.
 * Added Rb tag from Laurent.
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          | 53 ++++++++++++++-----
 1 file changed, 40 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
index 990e9c1dbc43..7a8f32473852 100644
--- a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
+++ b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
@@ -17,6 +17,7 @@ description:
 properties:
   compatible:
     enum:
+      - renesas,r9a07g044-vsp2 # RZ/G2L
       - renesas,vsp1 # R-Car Gen2 and RZ/G1
       - renesas,vsp2 # R-Car Gen3 and RZ/G2
 
@@ -26,8 +27,8 @@ properties:
   interrupts:
     maxItems: 1
 
-  clocks:
-    maxItems: 1
+  clocks: true
+  clock-names: true
 
   power-domains:
     maxItems: 1
@@ -50,17 +51,43 @@ 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,r9a07g044-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
+        clock-names: false
 
 examples:
   # R8A7790 (R-Car H2) VSP1-S
-- 
2.25.1

[PATCH v11 2/5] 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>
---
v10->v11:
 * To avoid lock-up on R-Car Gen2, added poll for reset status after deassert.
v9->v10:
 * Moved {deassert,assert} calls to vsp1_pm_runtime_{resume,suspend}
v8->v9:
 * No change
v7->v8:
 * No Change
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    | 32 +++++++++++++++++--
 2 files changed, 32 insertions(+), 2 deletions(-)

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 1f73c48eb738..466826db29f9 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
@@ -11,11 +11,13 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/interrupt.h>
+#include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #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>
@@ -622,6 +624,7 @@ static int __maybe_unused vsp1_pm_runtime_suspend(struct device *dev)
 	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
 
 	rcar_fcp_disable(vsp1->fcp);
+	reset_control_assert(vsp1->rstc);
 
 	return 0;
 }
@@ -631,13 +634,33 @@ static int __maybe_unused vsp1_pm_runtime_resume(struct device *dev)
 	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
 	int ret;
 
+	ret = reset_control_deassert(vsp1->rstc);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * On R-Car Gen2, vsp1 register access after deassert can cause
+	 * lock-up. Therefore, we need to poll the status of the reset to
+	 * avoid lock-up.
+	 */
+	ret = read_poll_timeout_atomic(reset_control_status, ret, ret == 0, 1,
+				       100, false, vsp1->rstc);
+	if (ret < 0)
+		goto done;
+
 	if (vsp1->info) {
 		ret = vsp1_device_init(vsp1);
 		if (ret < 0)
-			return ret;
+			goto done;
 	}
 
-	return rcar_fcp_enable(vsp1->fcp);
+	ret = rcar_fcp_enable(vsp1->fcp);
+
+done:
+	if (ret < 0)
+		reset_control_assert(vsp1->rstc);
+
+	return ret;
 }
 
 static const struct dev_pm_ops vsp1_pm_ops = {
@@ -825,6 +848,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 v11 3/5] media: renesas: vsp1: Add support for VSP software version

[Biju Das]

The VSPD block on RZ/G2L SoCs does not have a version register.

This patch adds support for adding VSP software version based on
device match.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v10->v11:
 * No change
v9->v10:
 * No change
v8->v9:
 * Added Rb tag from Geert
 * Updated commit description RZ/G2L -> RZ/G2L SoCs.
 * Replaced break with return info in case a Model match is found and
   removed additional check for non-match case.
v8:
 * New patch
---
 drivers/media/platform/renesas/vsp1/vsp1.h    |  1 +
 .../media/platform/renesas/vsp1/vsp1_drv.c    | 43 +++++++++++++------
 .../media/platform/renesas/vsp1/vsp1_regs.h   |  2 +
 3 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/renesas/vsp1/vsp1.h b/drivers/media/platform/renesas/vsp1/vsp1.h
index baf898d577ec..ff4435705abb 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1.h
+++ b/drivers/media/platform/renesas/vsp1/vsp1.h
@@ -67,6 +67,7 @@ struct vsp1_device_info {
 	unsigned int uif_count;
 	unsigned int wpf_count;
 	unsigned int num_bru_inputs;
+	u8 soc;
 	bool uapi;
 };
 
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
index 466826db29f9..43e3740bb041 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
@@ -821,11 +821,39 @@ static const struct vsp1_device_info vsp1_device_infos[] = {
 	},
 };
 
+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->version = VI6_IP_VERSION_VSP_SW | info->version | info->soc;
+		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)
+			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;
 
@@ -881,19 +909,8 @@ 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;
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_regs.h b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
index fae7286eb01e..4286d13eca32 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_regs.h
+++ b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
@@ -781,6 +781,8 @@
 #define VI6_IP_VERSION_SOC_E3		(0x04 << 0)
 #define VI6_IP_VERSION_SOC_V3U		(0x05 << 0)
 
+#define VI6_IP_VERSION_VSP_SW		(0xfffe << 16) /* SW VSP version */
+
 /* -----------------------------------------------------------------------------
  * RPF CLUT Registers
  */
-- 
2.25.1

[PATCH v11 4/5] media: renesas: vsp1: Add VSP1_HAS_NON_ZERO_LBA feature bit

[Biju Das]

As per HW manual V3M and RZ/G2L SoCs has nonzero LIF buffer
attributes. So, introduce a feature bit for handling the same.

This patch also adds separate device info structure for V3M and V3H
SoCs, as both these SoCs share the same model ID, but V3H does not
have VSP1_HAS_NON_ZERO_LBA feature bit.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v10->v11:
 * No change
v9->v10:
 * No change
v8->v9:
 * Used generic check for matching SoCs with LBA feature.
v8:
 * New patch
---
 drivers/media/platform/renesas/vsp1/vsp1.h     |  1 +
 drivers/media/platform/renesas/vsp1/vsp1_drv.c | 18 +++++++++++++++++-
 drivers/media/platform/renesas/vsp1/vsp1_lif.c |  3 +--
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/renesas/vsp1/vsp1.h b/drivers/media/platform/renesas/vsp1/vsp1.h
index ff4435705abb..2f6f0c6ae555 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1.h
+++ b/drivers/media/platform/renesas/vsp1/vsp1.h
@@ -55,6 +55,7 @@ struct vsp1_uif;
 #define VSP1_HAS_HGT		BIT(8)
 #define VSP1_HAS_BRS		BIT(9)
 #define VSP1_HAS_EXT_DL		BIT(10)
+#define VSP1_HAS_NON_ZERO_LBA	BIT(11)
 
 struct vsp1_device_info {
 	u32 version;
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
index 43e3740bb041..256794c67e63 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
@@ -791,6 +791,7 @@ static const struct vsp1_device_info vsp1_device_infos[] = {
 	}, {
 		.version = VI6_IP_VERSION_MODEL_VSPD_V3,
 		.model = "VSP2-D",
+		.soc = VI6_IP_VERSION_SOC_V3H,
 		.gen = 3,
 		.features = VSP1_HAS_BRS | VSP1_HAS_BRU,
 		.lif_count = 1,
@@ -798,6 +799,17 @@ static const struct vsp1_device_info vsp1_device_infos[] = {
 		.uif_count = 1,
 		.wpf_count = 1,
 		.num_bru_inputs = 5,
+	}, {
+		.version = VI6_IP_VERSION_MODEL_VSPD_V3,
+		.model = "VSP2-D",
+		.soc = VI6_IP_VERSION_SOC_V3M,
+		.gen = 3,
+		.features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_NON_ZERO_LBA,
+		.lif_count = 1,
+		.rpf_count = 5,
+		.uif_count = 1,
+		.wpf_count = 1,
+		.num_bru_inputs = 5,
 	}, {
 		.version = VI6_IP_VERSION_MODEL_VSPDL_GEN3,
 		.model = "VSP2-DL",
@@ -841,8 +853,12 @@ static const struct vsp1_device_info *vsp1_lookup_info(struct vsp1_device *vsp1)
 	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_MODEL_MASK) == info->version) {
+			if (info->soc &&
+			    ((vsp1->version & VI6_IP_VERSION_SOC_MASK) != info->soc))
+				continue;
 			return info;
+		}
 	}
 
 	dev_err(vsp1->dev, "unsupported IP version 0x%08x\n", vsp1->version);
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_lif.c b/drivers/media/platform/renesas/vsp1/vsp1_lif.c
index 6a6857ac9327..9adb892edcdc 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_lif.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_lif.c
@@ -135,8 +135,7 @@ static void lif_configure_stream(struct vsp1_entity *entity,
 	 * 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 (vsp1_feature(entity->vsp1, VSP1_HAS_NON_ZERO_LBA))
 		vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
 			       VI6_LIF_LBA_LBA0 |
 			       (1536 << VI6_LIF_LBA_LBA1_SHIFT));
-- 
2.25.1

[PATCH v11 5/5] 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 SoCs does not have a version register, so
added a new compatible string "renesas,r9a07g044-vsp2" with a data
pointer containing the info structure. Also the reset line is shared
with the DU module.

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>
---
v10->v11:
 * No change.
v9->v10:
 * No change.
v8->v9:
 * Replaced the code comments RZ/G2L {SoC's,SoC} with RZ/G2L SoCs.
v7->v8:
 * Split the patch for adding s/w version, feature bit and RZ/G2L support
 * Added feature bit VSP1_HAS_NON_ZERO_LBA to device_info
 * Added .soc for RZ/G2L
 * Replaced the compatible "renesas,rzg2l-vsp2" -> "renesas,r9a07g044-vsp2"
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_drv.c  | 13 +++++++++++++
 drivers/media/platform/renesas/vsp1/vsp1_lif.c  |  9 +++++----
 drivers/media/platform/renesas/vsp1/vsp1_regs.h |  4 ++++
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
index 256794c67e63..dd37fe81c4c5 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
@@ -833,6 +833,18 @@ 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",
+	.soc = VI6_IP_VERSION_SOC_RZG2L,
+	.gen = 3,
+	.features = VSP1_HAS_BRS | VSP1_HAS_WPF_VFLIP | VSP1_HAS_EXT_DL
+		  | VSP1_HAS_NON_ZERO_LBA,
+	.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;
@@ -983,6 +995,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,r9a07g044-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 9adb892edcdc..186a5730e1e3 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,10 +131,10 @@ 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 (vsp1_feature(entity->vsp1, VSP1_HAS_NON_ZERO_LBA))
 		vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_regs.h b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
index 4286d13eca32..8928f4c6bb55 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 SoCs 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,8 @@
 #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 SoCs have no version register, So use 0x80 for SoC Identification */
+#define VI6_IP_VERSION_SOC_RZG2L	(0x80 << 0)
 
 #define VI6_IP_VERSION_VSP_SW		(0xfffe << 16) /* SW VSP version */
 
-- 
2.25.1

RE: [PATCH v11 0/5] Add support for RZ/G2L VSPD

[Biju Das]

Hi All,

Gentle ping. Are we happy with this patch series?

Cheers,
Biju

> Subject: [PATCH v11 0/5] Add support for RZ/G2L VSPD
> 
> 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,r9a07g044-vsp2" with a data pointer
> containing the info structure. Also the reset line is shared with the DU
> module.
> 
> This patch series is tested on RZ/G1N, RZ/G2M and RZ/G2L boards.
> 
> v10->v11:
>  * Added poll for reset status in order to avoid lock-up on R-Car Gen2
>  * with vsp register access after deassert.
> 
> v9->v10
>  * Moved {deassert,assert} calls to vsp1_pm_runtime_{resume,suspend}
> 
> V8->v9
>  * Added Rb tag from Geert for patch#3
>  * Replaced break with return info in case a Model match is found and
>    removed additional check for non-match case.
>  * Used generic check for matching SoCs with LBA feature.
>  * Replaced the code comments RZ/G2L {SoC's,SoC} with RZ/G2L SoCs.
> v7->v8:
>  * Split the patch for adding s/w version, feature bit and RZ/G2L
> support
>  * Added feature bit VSP1_HAS_NON_ZERO_LBA to device_info
>  * Added .soc for RZ/G2L
>  * Replaced the compatible "renesas,rzg2l-vsp2" -> "renesas,r9a07g044-
> vsp2"
>  * Updated Clock-names to false for non RZ/G2L SoC's on binding doc
>  * Added Rb tag from Laurent for bindings
> 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:
>  *
>  *
> 
> Biju Das (5):
>   media: dt-bindings: media: renesas,vsp1: Document RZ/G2L VSPD bindings
>   media: renesas: vsp1: Add support to deassert/assert reset line
>   media: renesas: vsp1: Add support for VSP software version
>   media: renesas: vsp1: Add VSP1_HAS_NON_ZERO_LBA feature bit
>   media: renesas: vsp1: Add support for RZ/G2L VSPD
> 
>  .../bindings/media/renesas,vsp1.yaml          |  53 ++++++---
>  drivers/media/platform/renesas/vsp1/vsp1.h    |   4 +
>  .../media/platform/renesas/vsp1/vsp1_drv.c    | 104 +++++++++++++++---
>  .../media/platform/renesas/vsp1/vsp1_lif.c    |  12 +-
>  .../media/platform/renesas/vsp1/vsp1_regs.h   |   6 +
>  5 files changed, 145 insertions(+), 34 deletions(-)
> 
> --
> 2.25.1

RE: [PATCH v11 0/5] Add support for RZ/G2L VSPD

[Biju Das]

Hi Mauro, Laurent and Kieran,

Gentle ping,

Are there any issues with this patch series for acceptance?

Please let me know. If needed, I can send new patch series/follow-up patches for the suggestions.

Cheers,
Biju

> -----Original Message-----
> From: Biju Das
> Sent: 20 June 2022 07:52
> To: Mauro Carvalho Chehab <mchehab@kernel.org>; Laurent Pinchart
> <laurent.pinchart@ideasonboard.com>; Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>; linux-media@vger.kernel.org;
> linux-renesas-soc@vger.kernel.org; Geert Uytterhoeven
> <geert+renesas@glider.be>; Chris Paterson <Chris.Paterson2@renesas.com>;
> Biju Das <biju.das@bp.renesas.com>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Subject: RE: [PATCH v11 0/5] Add support for RZ/G2L VSPD
> 
> Hi All,
> 
> Gentle ping. Are we happy with this patch series?
> 
> Cheers,
> Biju
> 
> > Subject: [PATCH v11 0/5] Add support for RZ/G2L VSPD
> >
> > 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,r9a07g044-vsp2" with a data pointer
> > containing the info structure. Also the reset line is shared with the
> > DU module.
> >
> > This patch series is tested on RZ/G1N, RZ/G2M and RZ/G2L boards.
> >
> > v10->v11:
> >  * Added poll for reset status in order to avoid lock-up on R-Car Gen2
> >  * with vsp register access after deassert.
> >
> > v9->v10
> >  * Moved {deassert,assert} calls to vsp1_pm_runtime_{resume,suspend}
> >
> > V8->v9
> >  * Added Rb tag from Geert for patch#3
> >  * Replaced break with return info in case a Model match is found and
> >    removed additional check for non-match case.
> >  * Used generic check for matching SoCs with LBA feature.
> >  * Replaced the code comments RZ/G2L {SoC's,SoC} with RZ/G2L SoCs.
> > v7->v8:
> >  * Split the patch for adding s/w version, feature bit and RZ/G2L
> > support
> >  * Added feature bit VSP1_HAS_NON_ZERO_LBA to device_info
> >  * Added .soc for RZ/G2L
> >  * Replaced the compatible "renesas,rzg2l-vsp2" -> "renesas,r9a07g044-
> > vsp2"
> >  * Updated Clock-names to false for non RZ/G2L SoC's on binding doc
> >  * Added Rb tag from Laurent for bindings
> > 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:
> >  *
> >  *
> >
> > Biju Das (5):
> >   media: dt-bindings: media: renesas,vsp1: Document RZ/G2L VSPD
> bindings
> >   media: renesas: vsp1: Add support to deassert/assert reset line
> >   media: renesas: vsp1: Add support for VSP software version
> >   media: renesas: vsp1: Add VSP1_HAS_NON_ZERO_LBA feature bit
> >   media: renesas: vsp1: Add support for RZ/G2L VSPD
> >
> >  .../bindings/media/renesas,vsp1.yaml          |  53 ++++++---
> >  drivers/media/platform/renesas/vsp1/vsp1.h    |   4 +
> >  .../media/platform/renesas/vsp1/vsp1_drv.c    | 104 +++++++++++++++---
> >  .../media/platform/renesas/vsp1/vsp1_lif.c    |  12 +-
> >  .../media/platform/renesas/vsp1/vsp1_regs.h   |   6 +
> >  5 files changed, 145 insertions(+), 34 deletions(-)
> >
> > --
> > 2.25.1

Re: [PATCH v11 2/5] media: renesas: vsp1: Add support to deassert/assert reset line

[Philipp Zabel]

Hi Biju,

On Tue, May 31, 2022 at 03:19:55PM +0100, Biju Das wrote:
> 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>
> ---
> v10->v11:
>  * To avoid lock-up on R-Car Gen2, added poll for reset status after deassert.

I didn't look at this earlier because of my preexisting R-b.
It looks to me like this should be moved into the reset driver.

[...]
> @@ -631,13 +634,33 @@ static int __maybe_unused vsp1_pm_runtime_resume(struct device *dev)
>  	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
>  	int ret;
>  
> +	ret = reset_control_deassert(vsp1->rstc);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * On R-Car Gen2, vsp1 register access after deassert can cause
> +	 * lock-up. Therefore, we need to poll the status of the reset to
> +	 * avoid lock-up.
> +	 */
> +	ret = read_poll_timeout_atomic(reset_control_status, ret, ret == 0, 1,
> +				       100, false, vsp1->rstc);

So the reset driver does not follow the reset API documentation ("After
calling this function, the reset is guaranteed to be deasserted." [1])?
If so, this status polling should be moved into the reset driver.

Also, why use the atomic poll variant here? As far as I can tell, this
driver doesn't call pm_runtime_irq_safe. The reset_control_deassert()
API does not guarantee that the driver implementation doesn't sleep,
either.

[1] https://docs.kernel.org/driver-api/reset.html?highlight=reset_control_deassert#c.reset_control_deassert

[...]
> @@ -825,6 +848,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");
> +

What about the other consumers of this shared reset? Don't they need
the status poll you added here as well?

regards
Philipp

RE: [PATCH v11 2/5] media: renesas: vsp1: Add support to deassert/assert reset line

[Biju Das]

Hi Phil,

Thanks for the feedback.

> Subject: Re: [PATCH v11 2/5] media: renesas: vsp1: Add support to
> deassert/assert reset line
> 
> Hi Biju,
> 
> On Tue, May 31, 2022 at 03:19:55PM +0100, Biju Das wrote:
> > 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>
> > ---
> > v10->v11:
> >  * To avoid lock-up on R-Car Gen2, added poll for reset status after
> deassert.
> 
> I didn't look at this earlier because of my preexisting R-b.
> It looks to me like this should be moved into the reset driver.

OK, sorry, I should have removed Rb tag while sending this patch.

> [...]
> > @@ -631,13 +634,33 @@ static int __maybe_unused
> vsp1_pm_runtime_resume(struct device *dev)
> >  	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> >  	int ret;
> >
> > +	ret = reset_control_deassert(vsp1->rstc);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/*
> > +	 * On R-Car Gen2, vsp1 register access after deassert can cause
> > +	 * lock-up. Therefore, we need to poll the status of the reset to
> > +	 * avoid lock-up.
> > +	 */
> > +	ret = read_poll_timeout_atomic(reset_control_status, ret, ret == 0,
> 1,
> > +				       100, false, vsp1->rstc);
> 
> So the reset driver does not follow the reset API documentation ("After
> calling this function, the reset is guaranteed to be deasserted." [1])?
> If so, this status polling should be moved into the reset driver.
>

Sure, will move it to reset driver. Geert also suggested same thing[1]

[1]
https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220504184406.93788-1-biju.das.jz@bp.renesas.com/

 
> Also, why use the atomic poll variant here? As far as I can tell, this
> driver doesn't call pm_runtime_irq_safe. The reset_control_deassert() API
> does not guarantee that the driver implementation doesn't sleep, either.

As per [1], I2C driver uses atomic one, so just used the same here.

OK, will use non atomic variant in deassert().

Do you recommend to fix the reset as well as per [1]?

> 
> [...]
> > @@ -825,6 +848,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");
> > +
> 
> What about the other consumers of this shared reset? Don't they need the
> status poll you added here as well?

This lockup issue happens only on Gen2 SoC's. Gen3 SoC's are not affected.

RZ/G2L SoC is Gen3 variant, and it is the only consumer for shared reset as reset lines are shared between DU and VSPD. Other SoC's have explicit reset for VSP.

Cheers,
Biju

Re: [PATCH v11 2/5] media: renesas: vsp1: Add support to deassert/assert reset line

[Geert Uytterhoeven]

Hi Biju,

On Wed, Jul 13, 2022 at 11:18 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v11 2/5] media: renesas: vsp1: Add support to
> > deassert/assert reset line
> >
> > On Tue, May 31, 2022 at 03:19:55PM +0100, Biju Das wrote:
> > > 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>
> > > ---
> > > v10->v11:
> > >  * To avoid lock-up on R-Car Gen2, added poll for reset status after
> > deassert.
> >
> > I didn't look at this earlier because of my preexisting R-b.
> > It looks to me like this should be moved into the reset driver.
>
> OK, sorry, I should have removed Rb tag while sending this patch.
>
> > [...]
> > > @@ -631,13 +634,33 @@ static int __maybe_unused
> > vsp1_pm_runtime_resume(struct device *dev)
> > >     struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> > >     int ret;
> > >
> > > +   ret = reset_control_deassert(vsp1->rstc);
> > > +   if (ret < 0)
> > > +           return ret;
> > > +
> > > +   /*
> > > +    * On R-Car Gen2, vsp1 register access after deassert can cause
> > > +    * lock-up. Therefore, we need to poll the status of the reset to
> > > +    * avoid lock-up.
> > > +    */
> > > +   ret = read_poll_timeout_atomic(reset_control_status, ret, ret == 0,
> > 1,
> > > +                                  100, false, vsp1->rstc);
> >
> > So the reset driver does not follow the reset API documentation ("After
> > calling this function, the reset is guaranteed to be deasserted." [1])?
> > If so, this status polling should be moved into the reset driver.
> >
>
> Sure, will move it to reset driver. Geert also suggested same thing[1]

Actually I suggested handling this in the VSP driver, as VSP seems
to be "special".

>
> [1]
> https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220504184406.93788-1-biju.das.jz@bp.renesas.com/
>
>
> > Also, why use the atomic poll variant here? As far as I can tell, this
> > driver doesn't call pm_runtime_irq_safe. The reset_control_deassert() API
> > does not guarantee that the driver implementation doesn't sleep, either.
>
> As per [1], I2C driver uses atomic one, so just used the same here.
>
> OK, will use non atomic variant in deassert().
>
> Do you recommend to fix the reset as well as per [1]?
>
> >
> > [...]
> > > @@ -825,6 +848,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");
> > > +
> >
> > What about the other consumers of this shared reset? Don't they need the
> > status poll you added here as well?
>
> This lockup issue happens only on Gen2 SoC's. Gen3 SoC's are not affected.

We are not sure about that.  On R-Car Gen3, accesses to registers
while a device is not clocked/ready usually do not cause an imprecise
external abort in Linux, unlike on R-Car Gen2.  But perhaps the
abort is caught by the firmware, and nullified?

> RZ/G2L SoC is Gen3 variant, and it is the only consumer for shared reset as reset lines are shared between DU and VSPD. Other SoC's have explicit reset for VSP.

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 v11 2/5] media: renesas: vsp1: Add support to deassert/assert reset line

[Philipp Zabel]

Hi Biju, Geert,

On Wed, Jul 13, 2022 at 11:27:56AM +0200, Geert Uytterhoeven wrote:
[...]
> Actually I suggested handling this in the VSP driver, as VSP seems
> to be "special".
> 
> >
> > [1]
> > https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220504184406.93788-1-biju.das.jz@bp.renesas.com/

So reset_control_status never actually returns 1 and the polling loop is
not necessary at all?

If it's just the status register read that fixes things for VSP, could
it be that the deasserting register write to the reset controller
and the following register writes to VSP are not ordered somewhere at
the interconnect and the read issued to the reset controller just
guarantees that order?

regards
Philipp

Re: [PATCH v11 2/5] media: renesas: vsp1: Add support to deassert/assert reset line

[Geert Uytterhoeven]

Hi Philipp,

On Wed, Jul 13, 2022 at 12:32 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> On Wed, Jul 13, 2022 at 11:27:56AM +0200, Geert Uytterhoeven wrote:
> [...]
> > Actually I suggested handling this in the VSP driver, as VSP seems
> > to be "special".
> >
> > >
> > > [1]
> > > https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220504184406.93788-1-biju.das.jz@bp.renesas.com/
>
> So reset_control_status never actually returns 1 and the polling loop is
> not necessary at all?
>
> If it's just the status register read that fixes things for VSP, could
> it be that the deasserting register write to the reset controller
> and the following register writes to VSP are not ordered somewhere at
> the interconnect and the read issued to the reset controller just
> guarantees that order?

The udelay() also works.

While the reset may be deasserted immediately (at the reset controller
level), the VSP may need some additional time to settle/initialize
(at the VSP level).

Reset is known to work on other blocks on the same SoC, so that's
why I suggested handling this in the VSP driver instead, like we
already do for i2c.

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 v11 2/5] media: renesas: vsp1: Add support to deassert/assert reset line

[Biju Das]

Hi Philipp and Geert,

> Subject: Re: [PATCH v11 2/5] media: renesas: vsp1: Add support to
> deassert/assert reset line
> 
> Hi Philipp,
> 
> On Wed, Jul 13, 2022 at 12:32 PM Philipp Zabel <p.zabel@pengutronix.de>
> wrote:
> > On Wed, Jul 13, 2022 at 11:27:56AM +0200, Geert Uytterhoeven wrote:
> > [...]
> > > Actually I suggested handling this in the VSP driver, as VSP seems
> > > to be "special".
> > >
> > > >
> > > > [1]
> >
> > So reset_control_status never actually returns 1 and the polling loop
> > is not necessary at all?
> >
> > If it's just the status register read that fixes things for VSP, could
> > it be that the deasserting register write to the reset controller and
> > the following register writes to VSP are not ordered somewhere at the
> > interconnect and the read issued to the reset controller just
> > guarantees that order?
> 
> The udelay() also works.
> 
> While the reset may be deasserted immediately (at the reset controller
> level), the VSP may need some additional time to settle/initialize (at
> the VSP level).
> 
> Reset is known to work on other blocks on the same SoC, so that's why I
> suggested handling this in the VSP driver instead, like we already do for
> i2c.

From the discussion, we agree that the current implementation is good.

Please correct me if my understanding is wrong.

Cheers,
Biju

Re: [PATCH v11 2/5] media: renesas: vsp1: Add support to deassert/assert reset line

[Philipp Zabel]

Hi,

On Mo, 2022-07-18 at 09:46 +0000, Biju Das wrote:
> Hi Philipp and Geert,
> 
> > Subject: Re: [PATCH v11 2/5] media: renesas: vsp1: Add support to
> > deassert/assert reset line
> > 
> > Hi Philipp,
> > 
> > On Wed, Jul 13, 2022 at 12:32 PM Philipp Zabel <p.zabel@pengutronix.de>
> > wrote:
> > > On Wed, Jul 13, 2022 at 11:27:56AM +0200, Geert Uytterhoeven wrote:
> > > [...]
> > > > Actually I suggested handling this in the VSP driver, as VSP seems
> > > > to be "special".
> > > > 
> > > > > 
> > > > > [1]
> > > 
> > > So reset_control_status never actually returns 1 and the polling loop
> > > is not necessary at all?
> > > 
> > > If it's just the status register read that fixes things for VSP, could
> > > it be that the deasserting register write to the reset controller and
> > > the following register writes to VSP are not ordered somewhere at the
> > > interconnect and the read issued to the reset controller just
> > > guarantees that order?
> > 
> > The udelay() also works.
> > 
> > While the reset may be deasserted immediately (at the reset controller
> > level), the VSP may need some additional time to settle/initialize (at
> > the VSP level).

^ this feels to me like we are blindly applying a workaround for an
unknown problem. Is there any way to find out what actually causes this
delay (or status readback) to be necessary? Is there something
documented, like a certain number of VSP clocks required to internally
propagate the reset?

> > 
> > Reset is known to work on other blocks on the same SoC, so that's why I
> > suggested handling this in the VSP driver instead, like we already do for
> > i2c.
> 
> From the discussion, we agree that the current implementation is good.
> 
> Please correct me if my understanding is wrong.

From my understanding, not quite. At least the timeout poll is
unnecessary and misleading. It suggests that reset_control_status()
could return 0 at this point, which would be a bug in the reset driver.

If reset_control_status() only ever returns 1 and the polling loop ends
in the first iteration, you can drop the loop and just read status
once. Or use udelay(), at this point I have not enough information to
understand which would be more appropriate.

regards
Philipp

RE: [PATCH v11 2/5] media: renesas: vsp1: Add support to deassert/assert reset line

[Biju Das]

Hi Philipp and Geert,

> Subject: Re: [PATCH v11 2/5] media: renesas: vsp1: Add support to
> deassert/assert reset line
> 
> Hi,
> 
> On Mo, 2022-07-18 at 09:46 +0000, Biju Das wrote:
> > Hi Philipp and Geert,
> >
> > > Subject: Re: [PATCH v11 2/5] media: renesas: vsp1: Add support to
> > > deassert/assert reset line
> > >
> > > Hi Philipp,
> > >
> > > On Wed, Jul 13, 2022 at 12:32 PM Philipp Zabel
> > > <p.zabel@pengutronix.de>
> > > wrote:
> > > > On Wed, Jul 13, 2022 at 11:27:56AM +0200, Geert Uytterhoeven wrote:
> > > > [...]
> > > > > Actually I suggested handling this in the VSP driver, as VSP
> > > > > seems to be "special".
> > > > >
> > > > > >
> > > > > > [1]
> > > >
> > > > So reset_control_status never actually returns 1 and the polling
> > > > loop is not necessary at all?
> > > >
> > > > If it's just the status register read that fixes things for VSP,
> > > > could it be that the deasserting register write to the reset
> > > > controller and the following register writes to VSP are not
> > > > ordered somewhere at the interconnect and the read issued to the
> > > > reset controller just guarantees that order?
> > >
> > > The udelay() also works.
> > >
> > > While the reset may be deasserted immediately (at the reset
> > > controller level), the VSP may need some additional time to
> > > settle/initialize (at the VSP level).
> 
> ^ this feels to me like we are blindly applying a workaround for an
> unknown problem. Is there any way to find out what actually causes this
> delay (or status readback) to be necessary? Is there something
> documented, like a certain number of VSP clocks required to internally
> propagate the reset?

OK.

> 
> > >
> > > Reset is known to work on other blocks on the same SoC, so that's
> > > why I suggested handling this in the VSP driver instead, like we
> > > already do for i2c.
> >
> > From the discussion, we agree that the current implementation is good.
> >
> > Please correct me if my understanding is wrong.
> 
> From my understanding, not quite. At least the timeout poll is
> unnecessary and misleading. It suggests that reset_control_status() could
> return 0 at this point, which would be a bug in the reset driver.
> 
> If reset_control_status() only ever returns 1 and the polling loop ends
> in the first iteration, you can drop the loop and just read status once.
> Or use udelay(), at this point I have not enough information to
> understand which would be more appropriate.

For RZ/G1N SoC's like Geert mentioned in [1], calling reset_control_status() only once fixes the 
issue. So strictly speaking polling is not required.

@Geert Uytterhoeven, Please share your opinion on this.

[1]
https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220504184406.93788-1-biju.das.jz@bp.renesas.com/

Cheers,
Biju

Re: [PATCH v11 2/5] media: renesas: vsp1: Add support to deassert/assert reset line

[Geert Uytterhoeven]

Hi Biju,

On Mon, Jul 18, 2022 at 12:12 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v11 2/5] media: renesas: vsp1: Add support to
> > deassert/assert reset line
> >
> > Hi,
> >
> > On Mo, 2022-07-18 at 09:46 +0000, Biju Das wrote:
> > > Hi Philipp and Geert,
> > >
> > > > Subject: Re: [PATCH v11 2/5] media: renesas: vsp1: Add support to
> > > > deassert/assert reset line
> > > >
> > > > Hi Philipp,
> > > >
> > > > On Wed, Jul 13, 2022 at 12:32 PM Philipp Zabel
> > > > <p.zabel@pengutronix.de>
> > > > wrote:
> > > > > On Wed, Jul 13, 2022 at 11:27:56AM +0200, Geert Uytterhoeven wrote:
> > > > > [...]
> > > > > > Actually I suggested handling this in the VSP driver, as VSP
> > > > > > seems to be "special".
> > > > > >
> > > > > > >
> > > > > > > [1]
> > > > >
> > > > > So reset_control_status never actually returns 1 and the polling
> > > > > loop is not necessary at all?
> > > > >
> > > > > If it's just the status register read that fixes things for VSP,
> > > > > could it be that the deasserting register write to the reset
> > > > > controller and the following register writes to VSP are not
> > > > > ordered somewhere at the interconnect and the read issued to the
> > > > > reset controller just guarantees that order?
> > > >
> > > > The udelay() also works.
> > > >
> > > > While the reset may be deasserted immediately (at the reset
> > > > controller level), the VSP may need some additional time to
> > > > settle/initialize (at the VSP level).
> >
> > ^ this feels to me like we are blindly applying a workaround for an
> > unknown problem. Is there any way to find out what actually causes this
> > delay (or status readback) to be necessary? Is there something
> > documented, like a certain number of VSP clocks required to internally
> > propagate the reset?
>
> OK.
>
> >
> > > >
> > > > Reset is known to work on other blocks on the same SoC, so that's
> > > > why I suggested handling this in the VSP driver instead, like we
> > > > already do for i2c.
> > >
> > > From the discussion, we agree that the current implementation is good.
> > >
> > > Please correct me if my understanding is wrong.
> >
> > From my understanding, not quite. At least the timeout poll is
> > unnecessary and misleading. It suggests that reset_control_status() could
> > return 0 at this point, which would be a bug in the reset driver.
> >
> > If reset_control_status() only ever returns 1 and the polling loop ends
> > in the first iteration, you can drop the loop and just read status once.
> > Or use udelay(), at this point I have not enough information to
> > understand which would be more appropriate.
>
> For RZ/G1N SoC's like Geert mentioned in [1], calling reset_control_status() only once fixes the
> issue. So strictly speaking polling is not required.
>
> @Geert Uytterhoeven, Please share your opinion on this.

According to that thread, it also works without reading the
register, when adding a small delay() (to the vsp driver)?

>
> [1]
> https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220504184406.93788-1-biju.das.jz@bp.renesas.com/

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 v11 2/5] media: renesas: vsp1: Add support to deassert/assert reset line

[Biju Das]

Hi Geert and Philipp,

> Subject: Re: [PATCH v11 2/5] media: renesas: vsp1: Add support to
> deassert/assert reset line
> 
> Hi Biju,
> 
> On Mon, Jul 18, 2022 at 12:12 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH v11 2/5] media: renesas: vsp1: Add support to
> > > deassert/assert reset line
> > >
> > > Hi,
> > >
> > > On Mo, 2022-07-18 at 09:46 +0000, Biju Das wrote:
> > > > Hi Philipp and Geert,
> > > >
> > > > > Subject: Re: [PATCH v11 2/5] media: renesas: vsp1: Add support
> > > > > to deassert/assert reset line
> > > > >
> > > > > Hi Philipp,
> > > > >
> > > > > On Wed, Jul 13, 2022 at 12:32 PM Philipp Zabel
> > > > > <p.zabel@pengutronix.de>
> > > > > wrote:
> > > > > > On Wed, Jul 13, 2022 at 11:27:56AM +0200, Geert Uytterhoeven
> wrote:
> > > > > > [...]
> > > > > > > Actually I suggested handling this in the VSP driver, as VSP
> > > > > > > seems to be "special".
> > > > > > >
> > > > > > > >
> > > > > > > > [1]
> > > > > >
> > > > > > So reset_control_status never actually returns 1 and the
> > > > > > polling loop is not necessary at all?
> > > > > >
> > > > > > If it's just the status register read that fixes things for
> > > > > > VSP, could it be that the deasserting register write to the
> > > > > > reset controller and the following register writes to VSP are
> > > > > > not ordered somewhere at the interconnect and the read issued
> > > > > > to the reset controller just guarantees that order?
> > > > >
> > > > > The udelay() also works.
> > > > >
> > > > > While the reset may be deasserted immediately (at the reset
> > > > > controller level), the VSP may need some additional time to
> > > > > settle/initialize (at the VSP level).
> > >
> > > ^ this feels to me like we are blindly applying a workaround for an
> > > unknown problem. Is there any way to find out what actually causes
> > > this delay (or status readback) to be necessary? Is there something
> > > documented, like a certain number of VSP clocks required to
> > > internally propagate the reset?
> >
> > OK.
> >
> > >
> > > > >
> > > > > Reset is known to work on other blocks on the same SoC, so
> > > > > that's why I suggested handling this in the VSP driver instead,
> > > > > like we already do for i2c.
> > > >
> > > > From the discussion, we agree that the current implementation is
> good.
> > > >
> > > > Please correct me if my understanding is wrong.
> > >
> > > From my understanding, not quite. At least the timeout poll is
> > > unnecessary and misleading. It suggests that reset_control_status()
> > > could return 0 at this point, which would be a bug in the reset
> driver.
> > >
> > > If reset_control_status() only ever returns 1 and the polling loop
> > > ends in the first iteration, you can drop the loop and just read
> status once.
> > > Or use udelay(), at this point I have not enough information to
> > > understand which would be more appropriate.
> >
> > For RZ/G1N SoC's like Geert mentioned in [1], calling
> > reset_control_status() only once fixes the issue. So strictly speaking
> polling is not required.
> >
> > @Geert Uytterhoeven, Please share your opinion on this.
> 
> According to that thread, it also works without reading the register,
> when adding a small delay() (to the vsp driver)?

What about using the below logic?

If ((reset_control_status() == 0) && udelay(x))
	dev_warn(dev, "Deassert failed");

This makes system wake up after STR faster compared to
udelay.

Currently with first status readback() is sufficient for fixing the issue.
But this logic is tested only on few boards.

Adding a warn message to this logic, to check if any of the boards is 
returning reset_control_status() as "0".

Are we ok with this? Please share your views.

Cheers,
Biju