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

[PATCH 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,vsp2-rzg2l" with a data pointer containing
the info structure. Also the reset line is shared with the DU module
so devm_reset_control_get_shared() call is used in case of RZ/G2L.

RFC->v1:
 * Added reset support as seperate 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: vsp1: Add support to deassert/assert reset line
  media: vsp1: Add support for RZ/G2L VSPD

 .../bindings/media/renesas,vsp1.yaml          | 52 ++++++++++++++-----
 drivers/media/platform/vsp1/vsp1.h            |  1 +
 drivers/media/platform/vsp1/vsp1_drv.c        | 51 +++++++++++++++---
 drivers/media/platform/vsp1/vsp1_lif.c        |  7 ++-
 drivers/media/platform/vsp1/vsp1_regs.h       |  1 +
 5 files changed, 90 insertions(+), 22 deletions(-)

-- 
2.17.1

[PATCH 1/3] media: dt-bindings: media: renesas,vsp1: Document RZ/{G2L,V2L} VSPD bindings

[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,vsp2-rzg2l' 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>
---
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..52e26eea4f70 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,vsp2-rzg2l # 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,vsp2-rzg2l
+    then:
+      properties:
+        clocks:
+          items:
+            - description: LCDC Main clock
+            - description: LCDC Register Access Clock
+            - description: LCDC Video Clock
+        clock-names:
+          items:
+            - const: du.0
+            - const: pclk
+            - const: vclk
+      required:
+        - clock-names
+    else:
+      properties:
+        clocks:
+          maxItems: 1
 
 examples:
   # R8A7790 (R-Car H2) VSP1-S
-- 
2.17.1

[PATCH 2/3] media: 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 when required.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
RFC->v1:
 * Added reset support as seperate 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/vsp1/vsp1.h     |  1 +
 drivers/media/platform/vsp1/vsp1_drv.c | 21 ++++++++++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/vsp1/vsp1.h b/drivers/media/platform/vsp1/vsp1.h
index 37cf33c7e6ca..c5da829c79b5 100644
--- a/drivers/media/platform/vsp1/vsp1.h
+++ b/drivers/media/platform/vsp1/vsp1.h
@@ -79,6 +79,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/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
index 502c7d9d6890..77da6a6732d8 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/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,19 @@ 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 = reset_control_deassert(vsp1->rstc);
+
+	if (ret < 0)
+		goto err;
+
+	ret = pm_runtime_resume_and_get(vsp1->dev);
+	if (ret < 0)
+		goto err;
+
+	return 0;
+err:
+	reset_control_assert(vsp1->rstc);
+	return ret;
 }
 
 /*
@@ -581,6 +594,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 +841,11 @@ static int vsp1_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
+	vsp1->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+	if (IS_ERR(vsp1->rstc))
+		return dev_err_probe(&pdev->dev, PTR_ERR(vsp1->rstc),
+				     "failed to get reset ctrl\n");
+
 	/* FCP (optional). */
 	fcp_node = of_parse_phandle(pdev->dev.of_node, "renesas,fcp", 0);
 	if (fcp_node) {
-- 
2.17.1

[PATCH 3/3] media: 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,vsp2-rzg2l" with a data pointer containing
the info structure. Also the reset line is shared with the DU module
so devm_reset_control_get_shared() call is used in case of RZ/G2L.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
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/vsp1/vsp1_drv.c  | 32 +++++++++++++++++++------
 drivers/media/platform/vsp1/vsp1_lif.c  |  7 ++++--
 drivers/media/platform/vsp1/vsp1_regs.h |  1 +
 3 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
index 77da6a6732d8..40c6d9290681 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -811,6 +811,14 @@ static const struct vsp1_device_info vsp1_device_infos[] = {
 		.uif_count = 2,
 		.wpf_count = 1,
 		.num_bru_inputs = 5,
+	}, {
+		.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,
 	},
 };
 
@@ -841,7 +849,14 @@ static int vsp1_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
-	vsp1->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+	vsp1->info = of_device_get_match_data(&pdev->dev);
+	if (vsp1->info) {
+		vsp1->version = vsp1->info->version;
+		vsp1->rstc = devm_reset_control_get_shared(&pdev->dev, NULL);
+	} else {
+		vsp1->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+	}
+
 	if (IS_ERR(vsp1->rstc))
 		return dev_err_probe(&pdev->dev, PTR_ERR(vsp1->rstc),
 				     "failed to get reset ctrl\n");
@@ -874,13 +889,15 @@ static int vsp1_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto done;
 
-	vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
+	if (!vsp1->info) {
+		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;
+		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;
+			}
 		}
 	}
 
@@ -943,6 +960,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,vsp2-rzg2l", .data = &vsp1_device_infos[14] },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, vsp1_of_match);
diff --git a/drivers/media/platform/vsp1/vsp1_lif.c b/drivers/media/platform/vsp1/vsp1_lif.c
index 6a6857ac9327..6e997653cfac 100644
--- a/drivers/media/platform/vsp1/vsp1_lif.c
+++ b/drivers/media/platform/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;
@@ -135,8 +136,10 @@ 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 (((entity->vsp1->version & VI6_IP_VERSION_MASK) ==
+	    (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M)) ||
+	    ((entity->vsp1->version & VI6_IP_VERSION_MASK) ==
+	     VI6_IP_VERSION_MODEL_VSPD_RZG2L))
 		vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
 			       VI6_LIF_LBA_LBA0 |
 			       (1536 << VI6_LIF_LBA_LBA1_SHIFT));
diff --git a/drivers/media/platform/vsp1/vsp1_regs.h b/drivers/media/platform/vsp1/vsp1_regs.h
index fae7286eb01e..12c5b09885dc 100644
--- a/drivers/media/platform/vsp1/vsp1_regs.h
+++ b/drivers/media/platform/vsp1/vsp1_regs.h
@@ -766,6 +766,7 @@
 #define VI6_IP_VERSION_MODEL_VSPD_V3	(0x18 << 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_RZG2L	(0x1b << 8)
 #define VI6_IP_VERSION_MODEL_VSPD_V3U	(0x1c << 8)
 
 #define VI6_IP_VERSION_SOC_MASK		(0xff << 0)
-- 
2.17.1

Re: [PATCH 1/3] media: dt-bindings: media: renesas,vsp1: Document RZ/{G2L,V2L} VSPD bindings

[Geert Uytterhoeven]

Hi Biju,

On Wed, Mar 9, 2022 at 8:45 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,vsp2-rzg2l' 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>

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,vsp2-rzg2l # RZ/G2L and RZ/V2L

Please follow the recommended order <vendor>,[<family>|<soc>-]<function>,
i.e. "renesas,rzg2l-vsp2".

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 1/3] media: dt-bindings: media: renesas,vsp1: Document RZ/{G2L,V2L} VSPD bindings

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH 1/3] media: dt-bindings: media: renesas,vsp1: Document
> RZ/{G2L,V2L} VSPD bindings
> 
> Hi Biju,
> 
> On Wed, Mar 9, 2022 at 8:45 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,vsp2-rzg2l' 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>
> 
> 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,vsp2-rzg2l # RZ/G2L and RZ/V2L
> 
> Please follow the recommended order <vendor>,[<family>|<soc>-]<function>,
> i.e. "renesas,rzg2l-vsp2".

Agreed. Will send v2 with this changes.

Cheers,
Biju

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

[Philipp Zabel]

Hi Biju,

On Mi, 2022-03-09 at 19:45 +0000, Biju Das wrote:
[...]
> @@ -569,7 +570,19 @@ 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 = reset_control_deassert(vsp1->rstc);
> +
> +       if (ret < 0)
> +               goto err;
> +
> +       ret = pm_runtime_resume_and_get(vsp1->dev);
> +       if (ret < 0)
> +               goto err;
> +
> +       return 0;
> +err:
> +       reset_control_assert(vsp1->rstc);
> +       return ret;
>  }
>  
>  /*
> @@ -581,6 +594,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);

Now the comment above doesn't fit anymore, since reset_control_assert
is not reference counted in case vsp1->rstc has exclusive control:

/*                                                                                                                                            
 * vsp1_device_put - Release the VSP1 device                                                                                                  
 *                                                                                                                                            
 * Decrement the VSP1 reference count and cleanup the device if the last                                                                      
 * reference is released.                                                                                                                     
 */  

regards
Philipp

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

[Geert Uytterhoeven]

Hi Biju,

On Wed, Mar 9, 2022 at 8:45 PM Biju Das <biju.das.jz@bp.renesas.com> 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,vsp2-rzg2l" with a data pointer containing
> the info structure. Also the reset line is shared with the DU module
> so devm_reset_control_get_shared() call is used in case of RZ/G2L.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> 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/

Thanks for the update!

> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c

> @@ -841,7 +849,14 @@ static int vsp1_probe(struct platform_device *pdev)
>         if (irq < 0)
>                 return irq;
>
> -       vsp1->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +       vsp1->info = of_device_get_match_data(&pdev->dev);
> +       if (vsp1->info) {
> +               vsp1->version = vsp1->info->version;
> +               vsp1->rstc = devm_reset_control_get_shared(&pdev->dev, NULL);
> +       } else {
> +               vsp1->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);

Making the reset control shared or exclusive dependent on the presence
of match data looks fragile to me.  I think you want to check the IP
version instead (ideally, the SoC, as this is an integration feature).
Or just make it shared unconditionally (in the previous patch)?

> +       }
> +
>         if (IS_ERR(vsp1->rstc))
>                 return dev_err_probe(&pdev->dev, PTR_ERR(vsp1->rstc),
>                                      "failed to get reset ctrl\n");
> @@ -874,13 +889,15 @@ static int vsp1_probe(struct platform_device *pdev)
>         if (ret < 0)
>                 goto done;
>
> -       vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> +       if (!vsp1->info) {
> +               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;
> +               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;
> +                       }
>                 }
>         }
>
> @@ -943,6 +960,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,vsp2-rzg2l", .data = &vsp1_device_infos[14] },
>         { },

Is VI6_IP_VERSION_MODEL_VSPD_RZG2L = 0x1b an official number?
If yes, it might make sense to change the compatible value to
"renesas,vsp2-0x1b".

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

[Biju Das]

Hi Philipp,

Thanks for the feedback.

> Subject: Re: [PATCH 2/3] media: vsp1: Add support to deassert/assert reset
> line
> 
> Hi Biju,
> 
> On Mi, 2022-03-09 at 19:45 +0000, Biju Das wrote:
> [...]
> > @@ -569,7 +570,19 @@ 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 = reset_control_deassert(vsp1->rstc);
> > +
> > +       if (ret < 0)
> > +               goto err;
> > +
> > +       ret = pm_runtime_resume_and_get(vsp1->dev);
> > +       if (ret < 0)
> > +               goto err;
> > +
> > +       return 0;
> > +err:
> > +       reset_control_assert(vsp1->rstc);
> > +       return ret;
> >  }
> >
> >  /*
> > @@ -581,6 +594,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);
> 
> Now the comment above doesn't fit anymore, since reset_control_assert is
> not reference counted in case vsp1->rstc has exclusive control:

Thanks for pointing out. I will send v2 with shared reset instead of exclusive
control to match with the comment above.

Moreover for next patch, we need to use shared reset as reset line is shared
with DU Module.

Cheers,
Biju

> 
> /*
>  * vsp1_device_put - Release the VSP1 device
>  *
>  * Decrement the VSP1 reference count and cleanup the device if the last
>  * reference is released.
>  */
> 
> regards
> Philipp

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

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH 3/3] media: vsp1: Add support for RZ/G2L VSPD
> 
> Hi Biju,
> 
> On Wed, Mar 9, 2022 at 8:45 PM Biju Das <biju.das.jz@bp.renesas.com>
> 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,vsp2-rzg2l" with a data pointer
> > containing the info structure. Also the reset line is shared with the
> > DU module so devm_reset_control_get_shared() call is used in case of
> RZ/G2L.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > RFC->v1:
> >  * Used data pointer containing info structure to retrieve version
> > information
> > RFC:
> >  *
> 
> Thanks for the update!
> 
> > --- a/drivers/media/platform/vsp1/vsp1_drv.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> 
> > @@ -841,7 +849,14 @@ static int vsp1_probe(struct platform_device *pdev)
> >         if (irq < 0)
> >                 return irq;
> >
> > -       vsp1->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> > +       vsp1->info = of_device_get_match_data(&pdev->dev);
> > +       if (vsp1->info) {
> > +               vsp1->version = vsp1->info->version;
> > +               vsp1->rstc = devm_reset_control_get_shared(&pdev->dev,
> NULL);
> > +       } else {
> > +               vsp1->rstc =
> > + devm_reset_control_get_exclusive(&pdev->dev, NULL);
> 
> Making the reset control shared or exclusive dependent on the presence of
> match data looks fragile to me.  I think you want to check the IP version
> instead (ideally, the SoC, as this is an integration feature).
> Or just make it shared unconditionally (in the previous patch)?

Agreed.

> 
> > +       }
> > +
> >         if (IS_ERR(vsp1->rstc))
> >                 return dev_err_probe(&pdev->dev, PTR_ERR(vsp1->rstc),
> >                                      "failed to get reset ctrl\n"); @@
> > -874,13 +889,15 @@ static int vsp1_probe(struct platform_device *pdev)
> >         if (ret < 0)
> >                 goto done;
> >
> > -       vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> > +       if (!vsp1->info) {
> > +               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;
> > +               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;
> > +                       }
> >                 }
> >         }
> >
> > @@ -943,6 +960,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,vsp2-rzg2l", .data =
> > + &vsp1_device_infos[14] },
> >         { },
> 
> Is VI6_IP_VERSION_MODEL_VSPD_RZG2L = 0x1b an official number?
> If yes, it might make sense to change the compatible value to
> "renesas,vsp2-0x1b".

No, it is not official one. I just use 0x1b as no one claimed it.

Cheers,
Biju

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

[Kieran Bingham]

Hi Biju,

Quoting Biju Das (2022-03-09 19:45:21)
> 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,vsp2-rzg2l" with a data pointer containing

Does this mean it is 'not' a VSP2? Is it a VSP2-lite or something
different? (As opposed to 'the vsp2 found in an rzg2l part').


> the info structure. Also the reset line is shared with the DU module
> so devm_reset_control_get_shared() call is used in case of RZ/G2L.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> 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/vsp1/vsp1_drv.c  | 32 +++++++++++++++++++------
>  drivers/media/platform/vsp1/vsp1_lif.c  |  7 ++++--
>  drivers/media/platform/vsp1/vsp1_regs.h |  1 +
>  3 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
> index 77da6a6732d8..40c6d9290681 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -811,6 +811,14 @@ static const struct vsp1_device_info vsp1_device_infos[] = {
>                 .uif_count = 2,
>                 .wpf_count = 1,
>                 .num_bru_inputs = 5,
> +       }, {
> +               .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,
>         },
>  };
>  
> @@ -841,7 +849,14 @@ static int vsp1_probe(struct platform_device *pdev)
>         if (irq < 0)
>                 return irq;
>  
> -       vsp1->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +       vsp1->info = of_device_get_match_data(&pdev->dev);
> +       if (vsp1->info) {
> +               vsp1->version = vsp1->info->version;
> +               vsp1->rstc = devm_reset_control_get_shared(&pdev->dev, NULL);
> +       } else {
> +               vsp1->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +       }
> +

I'll leave this as Geert has already commented.

>         if (IS_ERR(vsp1->rstc))
>                 return dev_err_probe(&pdev->dev, PTR_ERR(vsp1->rstc),
>                                      "failed to get reset ctrl\n");
> @@ -874,13 +889,15 @@ static int vsp1_probe(struct platform_device *pdev)
>         if (ret < 0)
>                 goto done;
>  
> -       vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> +       if (!vsp1->info) {
> +               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;
> +               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;
> +                       }


This is looking like it gets a bit awkward. Two methods for identifying
the version and info structure is going to be a pain.


>                 }
>         }
>  
> @@ -943,6 +960,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,vsp2-rzg2l", .data = &vsp1_device_infos[14] },

I don't think you should reference a specific index of the infos table.
What happens if someone adds an entry higher in the table which pushes
the indexes down ?


>         { },
>  };
>  MODULE_DEVICE_TABLE(of, vsp1_of_match);
> diff --git a/drivers/media/platform/vsp1/vsp1_lif.c b/drivers/media/platform/vsp1/vsp1_lif.c
> index 6a6857ac9327..6e997653cfac 100644
> --- a/drivers/media/platform/vsp1/vsp1_lif.c
> +++ b/drivers/media/platform/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;
> @@ -135,8 +136,10 @@ 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 (((entity->vsp1->version & VI6_IP_VERSION_MASK) ==
> +           (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M)) ||
> +           ((entity->vsp1->version & VI6_IP_VERSION_MASK) ==
> +            VI6_IP_VERSION_MODEL_VSPD_RZG2L))

The comment here directly references V3M, and you haven't updated it.
But if this is going to grow I wonder if it will end up needing a quirks
flag that can be set per device in the vsp1_device_info rather than
coding a massive conditional if (platform x or platform y or platform
z.3);

>                 vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
>                                VI6_LIF_LBA_LBA0 |
>                                (1536 << VI6_LIF_LBA_LBA1_SHIFT));
> diff --git a/drivers/media/platform/vsp1/vsp1_regs.h b/drivers/media/platform/vsp1/vsp1_regs.h
> index fae7286eb01e..12c5b09885dc 100644
> --- a/drivers/media/platform/vsp1/vsp1_regs.h
> +++ b/drivers/media/platform/vsp1/vsp1_regs.h
> @@ -766,6 +766,7 @@
>  #define VI6_IP_VERSION_MODEL_VSPD_V3   (0x18 << 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_RZG2L        (0x1b << 8)

I don't like the idea of using a value here that could really be used on
a real device somewhere.

The hole in the sequence is only there because we havent' seen a
datasheet with 0x1b defined.

If there truely is no version register on this hardware, we're going to
have to make sure this version value can't conflict.

--
Kieran


>  #define VI6_IP_VERSION_MODEL_VSPD_V3U  (0x1c << 8)
>  
>  #define VI6_IP_VERSION_SOC_MASK                (0xff << 0)
> -- 
> 2.17.1
>

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

[Biju Das]

Hi Kieran,

Thanks for the feedback.

> Subject: Re: [PATCH 3/3] media: vsp1: Add support for RZ/G2L VSPD
> 
> Hi Biju,
> 
> Quoting Biju Das (2022-03-09 19:45:21)
> > 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,vsp2-rzg2l" with a data pointer
> > containing
> 
> Does this mean it is 'not' a VSP2? Is it a VSP2-lite or something
> different? (As opposed to 'the vsp2 found in an rzg2l part').

It is just VSPD, see the hardware manual[1]. It does not mention about VSP2-lite.

[1] https://www.renesas.com/document/mah/rzg2l-group-rzg2lc-group-users-manual-hardware-0?r=1467981

> 
> 
> > the info structure. Also the reset line is shared with the DU module
> > so devm_reset_control_get_shared() call is used in case of RZ/G2L.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > RFC->v1:
> >  * Used data pointer containing info structure to retrieve version
> > information
> > RFC:
> >  *
> > ---
> >  drivers/media/platform/vsp1/vsp1_drv.c  | 32
> > +++++++++++++++++++------  drivers/media/platform/vsp1/vsp1_lif.c  |
> > 7 ++++--  drivers/media/platform/vsp1/vsp1_regs.h |  1 +
> >  3 files changed, 31 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
> > b/drivers/media/platform/vsp1/vsp1_drv.c
> > index 77da6a6732d8..40c6d9290681 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drv.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> > @@ -811,6 +811,14 @@ static const struct vsp1_device_info
> vsp1_device_infos[] = {
> >                 .uif_count = 2,
> >                 .wpf_count = 1,
> >                 .num_bru_inputs = 5,
> > +       }, {
> > +               .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,
> >         },
> >  };
> >
> > @@ -841,7 +849,14 @@ static int vsp1_probe(struct platform_device *pdev)
> >         if (irq < 0)
> >                 return irq;
> >
> > -       vsp1->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> > +       vsp1->info = of_device_get_match_data(&pdev->dev);
> > +       if (vsp1->info) {
> > +               vsp1->version = vsp1->info->version;
> > +               vsp1->rstc = devm_reset_control_get_shared(&pdev->dev,
> NULL);
> > +       } else {
> > +               vsp1->rstc = devm_reset_control_get_exclusive(&pdev-
> >dev, NULL);
> > +       }
> > +
> 
> I'll leave this as Geert has already commented.
> 
> >         if (IS_ERR(vsp1->rstc))
> >                 return dev_err_probe(&pdev->dev, PTR_ERR(vsp1->rstc),
> >                                      "failed to get reset ctrl\n"); @@
> > -874,13 +889,15 @@ static int vsp1_probe(struct platform_device *pdev)
> >         if (ret < 0)
> >                 goto done;
> >
> > -       vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> > +       if (!vsp1->info) {
> > +               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;
> > +               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;
> > +                       }
> 
> 
> This is looking like it gets a bit awkward. Two methods for identifying
> the version and info structure is going to be a pain.

On RFC, Laurent suggested to use info for RZ/G2L. Do you have better 
Suggestion? Please let me know.

> 
> 
> >                 }
> >         }
> >
> > @@ -943,6 +960,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,vsp2-rzg2l", .data =
> > + &vsp1_device_infos[14] },
> 
> I don't think you should reference a specific index of the infos table.
> What happens if someone adds an entry higher in the table which pushes the
> indexes down ?

I can think of adding macros in info structure and use that macro
here to avoid such condition, if it all needed.

Do you have any other better alternative to handle this scenario?
Please let me know.

> 
> 
> >         { },
> >  };
> >  MODULE_DEVICE_TABLE(of, vsp1_of_match); diff --git
> > a/drivers/media/platform/vsp1/vsp1_lif.c
> > b/drivers/media/platform/vsp1/vsp1_lif.c
> > index 6a6857ac9327..6e997653cfac 100644
> > --- a/drivers/media/platform/vsp1/vsp1_lif.c
> > +++ b/drivers/media/platform/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;
> > @@ -135,8 +136,10 @@ 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 (((entity->vsp1->version & VI6_IP_VERSION_MASK) ==
> > +           (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M)) ||
> > +           ((entity->vsp1->version & VI6_IP_VERSION_MASK) ==
> > +            VI6_IP_VERSION_MODEL_VSPD_RZG2L))
> 
> The comment here directly references V3M, and you haven't updated it.
> But if this is going to grow I wonder if it will end up needing a quirks
> flag that can be set per device in the vsp1_device_info rather than coding
> a massive conditional if (platform x or platform y or platform z.3);

OK I can update the comment above in next version. Currently only
V2M and RZ/G2L need this change. In future if it all grows, we can
revisit and add the flags in info structure.

Are you ok with it? Please let me know.

> 
> >                 vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
> >                                VI6_LIF_LBA_LBA0 |
> >                                (1536 << VI6_LIF_LBA_LBA1_SHIFT)); diff
> > --git a/drivers/media/platform/vsp1/vsp1_regs.h
> > b/drivers/media/platform/vsp1/vsp1_regs.h
> > index fae7286eb01e..12c5b09885dc 100644
> > --- a/drivers/media/platform/vsp1/vsp1_regs.h
> > +++ b/drivers/media/platform/vsp1/vsp1_regs.h
> > @@ -766,6 +766,7 @@
> >  #define VI6_IP_VERSION_MODEL_VSPD_V3   (0x18 << 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_RZG2L        (0x1b << 8)
> 
> I don't like the idea of using a value here that could really be used on a
> real device somewhere.
> 
> The hole in the sequence is only there because we havent' seen a datasheet
> with 0x1b defined.
> 
> If there truely is no version register on this hardware, we're going to
> have to make sure this version value can't conflict.

Currently, I don't see any device with 0x1b. If in future if we found a device
With 0x1b, This can be moved to a higher value for eg:- 0xfe.

Please let me know your thoughts.

Cheers,
biju

> 
> 
> >  #define VI6_IP_VERSION_MODEL_VSPD_V3U  (0x1c << 8)
> >
> >  #define VI6_IP_VERSION_SOC_MASK                (0xff << 0)
> > --
> > 2.17.1
> >

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

[Geert Uytterhoeven]

Hi Biju,

On Thu, Mar 10, 2022 at 12:11 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH 3/3] media: vsp1: Add support for RZ/G2L VSPD
> > Quoting Biju Das (2022-03-09 19:45:21)
> > > 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,vsp2-rzg2l" with a data pointer
> > > containing

> > >  drivers/media/platform/vsp1/vsp1_drv.c  | 32
> > > +++++++++++++++++++------  drivers/media/platform/vsp1/vsp1_lif.c  |
> > > 7 ++++--  drivers/media/platform/vsp1/vsp1_regs.h |  1 +
> > >  3 files changed, 31 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
> > > b/drivers/media/platform/vsp1/vsp1_drv.c
> > > index 77da6a6732d8..40c6d9290681 100644
> > > --- a/drivers/media/platform/vsp1/vsp1_drv.c
> > > +++ b/drivers/media/platform/vsp1/vsp1_drv.c

> > >         if (IS_ERR(vsp1->rstc))
> > >                 return dev_err_probe(&pdev->dev, PTR_ERR(vsp1->rstc),
> > >                                      "failed to get reset ctrl\n"); @@
> > > -874,13 +889,15 @@ static int vsp1_probe(struct platform_device *pdev)
> > >         if (ret < 0)
> > >                 goto done;
> > >
> > > -       vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> > > +       if (!vsp1->info) {
> > > +               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;
> > > +               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;
> > > +                       }
> >
> >
> > This is looking like it gets a bit awkward. Two methods for identifying
> > the version and info structure is going to be a pain.
>
> On RFC, Laurent suggested to use info for RZ/G2L. Do you have better
> Suggestion? Please let me know.

I'm afraid we have no other option.  But the flow could be made
prettier by moving the table lookup into its own function, and using
something like below in the probe function:

    vsp1->info = of_device_get_match_data(&pdev->dev);
    if (!vsp1->info)
            vsp1->info = vsp1_lookup(vsp1_read(vsp1, VI6_IP_VERSION));
    if (!vsp1->info)
            return -ENODEV.

> > > @@ -943,6 +960,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,vsp2-rzg2l", .data =
> > > + &vsp1_device_infos[14] },
> >
> > I don't think you should reference a specific index of the infos table.
> > What happens if someone adds an entry higher in the table which pushes the
> > indexes down ?
>
> I can think of adding macros in info structure and use that macro
> here to avoid such condition, if it all needed.
>
> Do you have any other better alternative to handle this scenario?
> Please let me know.

I would use a pointer to an independent struct vsp1_device_info, not
part of vsp1_device_infos[], so it can never be matched by accident
(see below).

> > > --- a/drivers/media/platform/vsp1/vsp1_regs.h
> > > +++ b/drivers/media/platform/vsp1/vsp1_regs.h
> > > @@ -766,6 +766,7 @@
> > >  #define VI6_IP_VERSION_MODEL_VSPD_V3   (0x18 << 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_RZG2L        (0x1b << 8)
> >
> > I don't like the idea of using a value here that could really be used on a
> > real device somewhere.
> >
> > The hole in the sequence is only there because we havent' seen a datasheet
> > with 0x1b defined.
> >
> > If there truely is no version register on this hardware, we're going to
> > have to make sure this version value can't conflict.
>
> Currently, I don't see any device with 0x1b. If in future if we found a device
> With 0x1b, This can be moved to a higher value for eg:- 0xfe.
>
> Please let me know your thoughts.

I agree with Kieran, and strongly recommend against using a number
that might exist for real on current or future SoCs.  Unfortunately
there's only 8 bits available, precluding the use of e.g. (0xbeef01
<< 8). But starting from (0xff << 8), and counting down for future
entries, if needed, sounds like a good compromise.

And of course there should be a comment next to the definition,
to make it clear this is a made-up number.

P.S. If possible, please communicate to the hardware engineers it
     was IMHO a bad decision to get rid of the version register,
     which should be reconsidered for future SoCs.

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 3/3] media: vsp1: Add support for RZ/G2L VSPD

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH 3/3] media: vsp1: Add support for RZ/G2L VSPD
> 
> Hi Biju,
> 
> On Thu, Mar 10, 2022 at 12:11 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH 3/3] media: vsp1: Add support for RZ/G2L VSPD
> > > Quoting Biju Das (2022-03-09 19:45:21)
> > > > 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,vsp2-rzg2l" with a data
> > > > pointer containing
> 
> > > >  drivers/media/platform/vsp1/vsp1_drv.c  | 32
> > > > +++++++++++++++++++------  drivers/media/platform/vsp1/vsp1_lif.c
> > > > +++++++++++++++++++|
> > > > 7 ++++--  drivers/media/platform/vsp1/vsp1_regs.h |  1 +
> > > >  3 files changed, 31 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
> > > > b/drivers/media/platform/vsp1/vsp1_drv.c
> > > > index 77da6a6732d8..40c6d9290681 100644
> > > > --- a/drivers/media/platform/vsp1/vsp1_drv.c
> > > > +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> 
> > > >         if (IS_ERR(vsp1->rstc))
> > > >                 return dev_err_probe(&pdev->dev, PTR_ERR(vsp1-
> >rstc),
> > > >                                      "failed to get reset
> > > > ctrl\n"); @@
> > > > -874,13 +889,15 @@ static int vsp1_probe(struct platform_device
> *pdev)
> > > >         if (ret < 0)
> > > >                 goto done;
> > > >
> > > > -       vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> > > > +       if (!vsp1->info) {
> > > > +               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;
> > > > +               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;
> > > > +                       }
> > >
> > >
> > > This is looking like it gets a bit awkward. Two methods for
> > > identifying the version and info structure is going to be a pain.
> >
> > On RFC, Laurent suggested to use info for RZ/G2L. Do you have better
> > Suggestion? Please let me know.
> 
> I'm afraid we have no other option.  But the flow could be made prettier
> by moving the table lookup into its own function, and using something like
> below in the probe function:

Sounds good to me. Kieran/Laurent, are you OK with Geert's suggestion?

> 
>     vsp1->info = of_device_get_match_data(&pdev->dev);
>     if (!vsp1->info)
>             vsp1->info = vsp1_lookup(vsp1_read(vsp1, VI6_IP_VERSION));
>     if (!vsp1->info)
>             return -ENODEV.
> 
> > > > @@ -943,6 +960,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,vsp2-rzg2l", .data =
> > > > + &vsp1_device_infos[14] },
> > >
> > > I don't think you should reference a specific index of the infos
> table.
> > > What happens if someone adds an entry higher in the table which
> > > pushes the indexes down ?
> >
> > I can think of adding macros in info structure and use that macro here
> > to avoid such condition, if it all needed.
> >
> > Do you have any other better alternative to handle this scenario?
> > Please let me know.
> 
> I would use a pointer to an independent struct vsp1_device_info, not part
> of vsp1_device_infos[], so it can never be matched by accident (see
> below).

Sounds good to me. Kieran/Laurent, are you OK with Geert's suggestion?

> 
> > > > --- a/drivers/media/platform/vsp1/vsp1_regs.h
> > > > +++ b/drivers/media/platform/vsp1/vsp1_regs.h
> > > > @@ -766,6 +766,7 @@
> > > >  #define VI6_IP_VERSION_MODEL_VSPD_V3   (0x18 << 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_RZG2L        (0x1b << 8)
> > >
> > > I don't like the idea of using a value here that could really be
> > > used on a real device somewhere.
> > >
> > > The hole in the sequence is only there because we havent' seen a
> > > datasheet with 0x1b defined.
> > >
> > > If there truely is no version register on this hardware, we're going
> > > to have to make sure this version value can't conflict.
> >
> > Currently, I don't see any device with 0x1b. If in future if we found
> > a device With 0x1b, This can be moved to a higher value for eg:- 0xfe.
> >
> > Please let me know your thoughts.
> 
> I agree with Kieran, and strongly recommend against using a number that
> might exist for real on current or future SoCs.  Unfortunately there's
> only 8 bits available, precluding the use of e.g. (0xbeef01 << 8). But
> starting from (0xff << 8), and counting down for future entries, if
> needed, sounds like a good compromise.
> 
> And of course there should be a comment next to the definition, to make it
> clear this is a made-up number.

Sounds good to me. Kieran/Laurent, are you OK with Geert's suggestion?

> 
> P.S. If possible, please communicate to the hardware engineers it
>      was IMHO a bad decision to get rid of the version register,
>      which should be reconsidered for future SoCs.

I am checking with HW engineer, I will update you once I get feedback from them.

Cheers,
Biju