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

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

[Biju Das]

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

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

v4->v5:
 * Fixed typo VI6_IP_VERSION_MODEL_MASK->VI6_IP_VERSION_MASK
 * To be consistent with other SoC's, introduced VI6_IP_VERSION_SOC_G2L
   for SoC identification for RZ/G2L SoC's.
v3->v4:
 * Restored error check for pm_runtime_resume_and_get and calls
   assert() in case of failure.
 * Added Rb tag from Geert
 * Add switch() for LIF0 buffer attribute handling for RZ/G2L and V3M SoC's
v2->v3:
 * Added Rb tags from Krzysztof and Philipp
 * If reset_control_deassert() failed, return ret directly.
 * Fixed version comparison in vsp1_lookup()
v1->v2:
 * Used reference counted reset handle to perform deassert/assert
 * Changed the compatible from vsp2-rzg2l->rzg2l-vsp2
 * Added standalone device info for rzg2l-vsp2.
 * Added vsp1_lookup helper function.
 * Updated comments for LIF0 buffer attribute register
 * Used last ID for rzg2l-vsp2.
RFC->v1:
 * Added reset support as separate patch
 * Moved rstc just after the bus_master field in struct vsp1_device
 * Used data pointer containing info structure to retrieve version information
 * Updated commit description
 * Changed compatible from vsp2-r9a07g044->vsp2-rzg2l
 * Defined the clocks
 * Clock max Items is based on SoC Compatible string

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

Biju Das (3):
  media: dt-bindings: media: renesas,vsp1: Document RZ/{G2L,V2L} VSPD
    bindings
  media: 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        | 62 +++++++++++++++----
 drivers/media/platform/vsp1/vsp1_lif.c        | 16 +++--
 drivers/media/platform/vsp1/vsp1_regs.h       |  4 ++
 5 files changed, 105 insertions(+), 30 deletions(-)

-- 
2.17.1

[PATCH v5 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,rzg2l-vsp2' to handle
these differences.

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

diff --git a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
index 990e9c1dbc43..2696a4582251 100644
--- a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
+++ b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
@@ -19,6 +19,7 @@ properties:
     enum:
       - renesas,vsp1 # R-Car Gen2 and RZ/G1
       - renesas,vsp2 # R-Car Gen3 and RZ/G2
+      - renesas,rzg2l-vsp2 # RZ/G2L and RZ/V2L
 
   reg:
     maxItems: 1
@@ -26,8 +27,8 @@ properties:
   interrupts:
     maxItems: 1
 
-  clocks:
-    maxItems: 1
+  clocks: true
+  clock-names: true
 
   power-domains:
     maxItems: 1
@@ -50,17 +51,42 @@ required:
 
 additionalProperties: false
 
-if:
-  properties:
-    compatible:
-      items:
-        - const: renesas,vsp1
-then:
-  properties:
-    renesas,fcp: false
-else:
-  required:
-    - renesas,fcp
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: renesas,vsp1
+    then:
+      properties:
+        renesas,fcp: false
+    else:
+      required:
+        - renesas,fcp
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: renesas,rzg2l-vsp2
+    then:
+      properties:
+        clocks:
+          items:
+            - description: 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

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

[Laurent Pinchart]

Hi Biju,

Thank you for the patch.

On Sat, Mar 12, 2022 at 08:42:03AM +0000, Biju Das wrote:
> Document VSPD found in RZ/G2L and RZ/V2L family SoC's. VSPD block is
> similar to VSP2-D found on R-Car SoC's, but it does not have a version
> register and it has 3 clocks compared to 1 clock on vsp1 and vsp2.
> 
> This patch introduces a new compatible 'renesas,rzg2l-vsp2' to handle
> these differences.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> ---
> v4->v5:
>  * No change
> v3->v4:
>  * No change
> v2->v3:
>  * Added Rb tag from Krzysztof.
> v1->v2:
>  * Changed compatible from vsp2-rzg2l->rzg2l-vsp2
> RFC->v1:
>  * Updated commit description
>  * Changed compatible from vsp2-r9a07g044->vsp2-rzg2l
>  * Defined the clocks
>  * Clock max Items is based on SoC Compatible string
> RFC:
>  * https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-20-biju.das.jz@bp.renesas.com/
> ---
>  .../bindings/media/renesas,vsp1.yaml          | 52 ++++++++++++++-----
>  1 file changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> index 990e9c1dbc43..2696a4582251 100644
> --- a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> +++ b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> @@ -19,6 +19,7 @@ properties:
>      enum:
>        - renesas,vsp1 # R-Car Gen2 and RZ/G1
>        - renesas,vsp2 # R-Car Gen3 and RZ/G2
> +      - renesas,rzg2l-vsp2 # RZ/G2L and RZ/V2L
>  
>    reg:
>      maxItems: 1
> @@ -26,8 +27,8 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> -  clocks:
> -    maxItems: 1
> +  clocks: true
> +  clock-names: true
>  
>    power-domains:
>      maxItems: 1
> @@ -50,17 +51,42 @@ required:
>  
>  additionalProperties: false
>  
> -if:
> -  properties:
> -    compatible:
> -      items:
> -        - const: renesas,vsp1
> -then:
> -  properties:
> -    renesas,fcp: false
> -else:
> -  required:
> -    - renesas,fcp
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: renesas,vsp1
> +    then:
> +      properties:
> +        renesas,fcp: false
> +    else:
> +      required:
> +        - renesas,fcp
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: renesas,rzg2l-vsp2
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: LCDC Main clock
> +            - description: LCDC Register Access Clock
> +            - description: LCDC Video Clock

Shouldn't we avoid referring to LCDC in the VSP bindings ?

> +        clock-names:
> +          items:
> +            - const: du.0

Similarly, I'm not sure this is a good name from the point of view of
the VSP.

> +            - const: pclk
> +            - const: vclk

I couldn't find those names in the documentation, where do they come
from ? Could you maybe share a DT integration example ?

> +      required:
> +        - clock-names
> +    else:
> +      properties:
> +        clocks:
> +          maxItems: 1
>  
>  examples:
>    # R8A7790 (R-Car H2) VSP1-S

-- 
Regards,

Laurent Pinchart

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

[Biju Das]

Hi Laurent,

Thanks for the feedback.

> Subject: Re: [PATCH v5 1/3] media: dt-bindings: media: renesas,vsp1:
> Document RZ/{G2L,V2L} VSPD bindings
> 
> Hi Biju,
> 
> Thank you for the patch.
> 
> On Sat, Mar 12, 2022 at 08:42:03AM +0000, Biju Das wrote:
> > Document VSPD found in RZ/G2L and RZ/V2L family SoC's. VSPD block is
> > similar to VSP2-D found on R-Car SoC's, but it does not have a version
> > register and it has 3 clocks compared to 1 clock on vsp1 and vsp2.
> >
> > This patch introduces a new compatible 'renesas,rzg2l-vsp2' to handle
> > these differences.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> > ---
> > v4->v5:
> >  * No change
> > v3->v4:
> >  * No change
> > v2->v3:
> >  * Added Rb tag from Krzysztof.
> > v1->v2:
> >  * Changed compatible from vsp2-rzg2l->rzg2l-vsp2
> > RFC->v1:
> >  * Updated commit description
> >  * Changed compatible from vsp2-r9a07g044->vsp2-rzg2l
> >  * Defined the clocks
> >  * Clock max Items is based on SoC Compatible string
> > RFC:
> >  *
> > ---
> >  .../bindings/media/renesas,vsp1.yaml          | 52 ++++++++++++++-----
> >  1 file changed, 39 insertions(+), 13 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > index 990e9c1dbc43..2696a4582251 100644
> > --- a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > +++ b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > @@ -19,6 +19,7 @@ properties:
> >      enum:
> >        - renesas,vsp1 # R-Car Gen2 and RZ/G1
> >        - renesas,vsp2 # R-Car Gen3 and RZ/G2
> > +      - renesas,rzg2l-vsp2 # RZ/G2L and RZ/V2L
> >
> >    reg:
> >      maxItems: 1
> > @@ -26,8 +27,8 @@ properties:
> >    interrupts:
> >      maxItems: 1
> >
> > -  clocks:
> > -    maxItems: 1
> > +  clocks: true
> > +  clock-names: true
> >
> >    power-domains:
> >      maxItems: 1
> > @@ -50,17 +51,42 @@ required:
> >
> >  additionalProperties: false
> >
> > -if:
> > -  properties:
> > -    compatible:
> > -      items:
> > -        - const: renesas,vsp1
> > -then:
> > -  properties:
> > -    renesas,fcp: false
> > -else:
> > -  required:
> > -    - renesas,fcp
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: renesas,vsp1
> > +    then:
> > +      properties:
> > +        renesas,fcp: false
> > +    else:
> > +      required:
> > +        - renesas,fcp
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: renesas,rzg2l-vsp2
> > +    then:
> > +      properties:
> > +        clocks:
> > +          items:
> > +            - description: LCDC Main clock
> > +            - description: LCDC Register Access Clock
> > +            - description: LCDC Video Clock
> 
> Shouldn't we avoid referring to LCDC in the VSP bindings ?

OK will drop prefix LCDC.

> 
> > +        clock-names:
> > +          items:
> > +            - const: du.0
> 
> Similarly, I'm not sure this is a good name from the point of view of the
> VSP.

OK, will use the name 'aclk', which is Main clock for this module which is
shared with LCDC. 'du.0' is not valid any more here as we are using different
CRTC implementation for RZ/G2LC.

> 
> > +            - const: pclk
> > +            - const: vclk
> 
> I couldn't find those names in the documentation, where do they come from

HW manual (page 312) mentions about LCDC_CLK_A, LCDC_CLK_P & LCDC_CLK_D.

Detailed description is mentioned in Clock list document. Please see below.

	LCDC_CLK_A	M0φ	PLL3	200	200		LCDC  Main clock
	LCDC_CLK_P	ZTφ	PLL3	100	100		LCDC Register Access Clock	
	LCDC_CLK_D	M3φ	SEL_PLL5_4	148.5~5.803	LCDC Video Clock	

> ? Could you maybe share a DT integration example ?

Please see below,

+		fcpvd0: fcp@10880000 {
+			compatible = "renesas,fcpv";
+			reg = <0 0x10880000 0 0x10000>;
+			clocks = <&cpg CPG_MOD R9A07G044_LCDC_CLK_A>,
+				 <&cpg CPG_MOD R9A07G044_LCDC_CLK_P>,
+				 <&cpg CPG_MOD R9A07G044_LCDC_CLK_D>;
+			power-domains = <&cpg>;
+			resets = <&cpg R9A07G044_LCDC_RESET_N>;
+		};

+		vspd0: vsp@10870000 {
+			compatible = "renesas,rzg2l-vsp2";
+			reg = <0 0x10870000 0 0x10000>;
+			interrupts = <GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A07G044_LCDC_CLK_A>,
+				 <&cpg CPG_MOD R9A07G044_LCDC_CLK_P>,
+				 <&cpg CPG_MOD R9A07G044_LCDC_CLK_D>;
+			clock-names = "du.0", "pclk", "vclk";
+			power-domains = <&cpg>;
+			resets = <&cpg R9A07G044_LCDC_RESET_N>;
+			renesas,fcp = <&fcpvd0>;
+		};

+		du: display@0x10890000 {
+			compatible = "renesas,du-r9a07g044l";
+			reg = <0 0x10890000 0 0x10000>;
+			interrupts = <GIC_SPI 152 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A07G044_LCDC_CLK_A>,
+				 <&cpg CPG_MOD R9A07G044_LCDC_CLK_P>,
+				 <&cpg CPG_MOD R9A07G044_LCDC_CLK_D>;
+			clock-names = "du.0", "pclk", "vclk";
+			power-domains = <&cpg>;
+			resets = <&cpg R9A07G044_LCDC_RESET_N>;
+			reset-names = "du.0";
+			renesas,vsps = <&vspd0 0>;
+
+			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@0 {
+					reg = <0>;
+					du_out_rgb: endpoint {
+					};
+				};
+
+				port@1 {
+					reg = <1>;
+					du_out_dsi0: endpoint {
+					};
+				};
+			};
+		};

+		dsi0: dsi@10860000 {
+			compatible = "renesas,r9a07g044-mipi-dsi";
+			reg =	<0 0x10860000 0 0x10000>, /* LINK */
+				<0 0x10850000 0 0x10000>; /* DPHY */
+
+			clocks = <&cpg CPG_MOD R9A07G044_MIPI_DSI_PLLCLK>,
+				 <&cpg CPG_MOD R9A07G044_MIPI_DSI_SYSCLK>,
+				 <&cpg CPG_MOD R9A07G044_MIPI_DSI_ACLK>,
+				 <&cpg CPG_MOD R9A07G044_MIPI_DSI_PCLK>,
+				 <&cpg CPG_MOD R9A07G044_MIPI_DSI_VCLK>,
+				 <&cpg CPG_MOD R9A07G044_MIPI_DSI_LPCLK>;
+			clock-names = "pllclk", "sysclk", "aclk", "pclk", "vclk", "lpclk";
+			power-domains = <&cpg>;
+			resets = <&cpg R9A07G044_MIPI_DSI_CMN_RSTB>,
+				 <&cpg R9A07G044_MIPI_DSI_ARESET_N>,
+				 <&cpg R9A07G044_MIPI_DSI_PRESET_N>;
+			reset-names = "rst", "arst", "prst";
+			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@0 {
+					reg = <0>;
+					dsi0_in: endpoint {
+						remote-endpoint = <&dsi0_in>;
+					};
+				};
+
+				port@1 {
+					reg = <1>;
+					dsi0_out: endpoint {
+						remote-endpoint = <&du_out_dsi0>;
+					};
+				};
+			};
+		};

+	hdmi-out {
+		compatible = "hdmi-connector";
+		type = "d";
+
+		port {
+			hdmi_con_out: endpoint {
+				remote-endpoint = <&adv7535_out>;
+			};
+		};
+	};

+&dsi0 {
+	status = "okay";
+
+	ports {
+		port@1 {
+			dsi0_out: endpoint {
+				data-lanes = <1 2 3 4>;
+				remote-endpoint = <&adv7535_in>;
+			};
+		};
+	};
+};

+&du {
+	status = "okay";
+};

+&i2c1 {
+	pinctrl-0 = <&i2c1_pins>;
+	pinctrl-names = "default";
+
+	status = "okay";
+
+	adv7535: hdmi@3d {
+		compatible = "adi,adv7535";
+		reg = <0x3d>;
+
+		avdd-supply = <&reg_1p8v>;
+		dvdd-supply = <&reg_1p8v>;
+		pvdd-supply = <&reg_1p8v>;
+		a2vdd-supply = <&reg_1p8v>;
+		v3p3-supply = <&reg_3p3v>;
+		v1p2-supply = <&reg_1p8v>;
+
+		adi,dsi-lanes = <4>;
+		adi,disable-lanes-override;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				adv7535_in: endpoint {
+					remote-endpoint = <&dsi0_out>;
+				};
+			};
+
+			port@1 {
+				reg = <1>;
+				adv7535_out: endpoint {
+					remote-endpoint = <&hdmi_con_out>;
+				};
+			};
+		};
+	};

Cheers,
Biju

> 
> > +      required:
> > +        - clock-names
> > +    else:
> > +      properties:
> > +        clocks:
> > +          maxItems: 1
> >
> >  examples:
> >    # R8A7790 (R-Car H2) VSP1-S
> 
> --
> Regards,
> 
> Laurent Pinchart

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

[Geert Uytterhoeven]

Hi Biju,

On Mon, Mar 14, 2022 at 9:44 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v5 1/3] media: dt-bindings: media: renesas,vsp1:
> > Document RZ/{G2L,V2L} VSPD bindings
> > On Sat, Mar 12, 2022 at 08:42:03AM +0000, Biju Das wrote:
> > > Document VSPD found in RZ/G2L and RZ/V2L family SoC's. VSPD block is
> > > similar to VSP2-D found on R-Car SoC's, but it does not have a version
> > > register and it has 3 clocks compared to 1 clock on vsp1 and vsp2.
> > >
> > > This patch introduces a new compatible 'renesas,rzg2l-vsp2' to handle
> > > these differences.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

> > > index 990e9c1dbc43..2696a4582251 100644
> > > --- a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > > +++ b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml

> > > +        clock-names:
> > > +          items:
> > > +            - const: du.0
> >
> > Similarly, I'm not sure this is a good name from the point of view of the
> > VSP.
>
> OK, will use the name 'aclk', which is Main clock for this module which is
> shared with LCDC. 'du.0' is not valid any more here as we are using different
> CRTC implementation for RZ/G2LC.
>
> >
> > > +            - const: pclk
> > > +            - const: vclk
> >
> > I couldn't find those names in the documentation, where do they come from
>
> HW manual (page 312) mentions about LCDC_CLK_A, LCDC_CLK_P & LCDC_CLK_D.
>
> Detailed description is mentioned in Clock list document. Please see below.
>
>         LCDC_CLK_A      M0φ     PLL3    200     200             LCDC  Main clock
>         LCDC_CLK_P      ZTφ     PLL3    100     100             LCDC Register Access Clock
>         LCDC_CLK_D      M3φ     SEL_PLL5_4      148.5~5.803     LCDC Video Clock
>
> > ? Could you maybe share a DT integration example ?
>
> Please see below,

>
> +               du: display@0x10890000 {
> +                       compatible = "renesas,du-r9a07g044l";
> +                       reg = <0 0x10890000 0 0x10000>;
> +                       interrupts = <GIC_SPI 152 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&cpg CPG_MOD R9A07G044_LCDC_CLK_A>,
> +                                <&cpg CPG_MOD R9A07G044_LCDC_CLK_P>,
> +                                <&cpg CPG_MOD R9A07G044_LCDC_CLK_D>;
> +                       clock-names = "du.0", "pclk", "vclk";
> +                       power-domains = <&cpg>;
> +                       resets = <&cpg R9A07G044_LCDC_RESET_N>;
> +                       reset-names = "du.0";
> +                       renesas,vsps = <&vspd0 0>;

Given the DU driver is no longer shared, perhaps all occurrencies of "du"
should be replaced by "lcdc"?

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 v5 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 v5 1/3] media: dt-bindings: media: renesas,vsp1:
> Document RZ/{G2L,V2L} VSPD bindings
> 
> Hi Biju,
> 
> On Mon, Mar 14, 2022 at 9:44 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH v5 1/3] media: dt-bindings: media: renesas,vsp1:
> > > Document RZ/{G2L,V2L} VSPD bindings
> > > On Sat, Mar 12, 2022 at 08:42:03AM +0000, Biju Das wrote:
> > > > Document VSPD found in RZ/G2L and RZ/V2L family SoC's. VSPD block
> > > > is similar to VSP2-D found on R-Car SoC's, but it does not have a
> > > > version register and it has 3 clocks compared to 1 clock on vsp1 and
> vsp2.
> > > >
> > > > This patch introduces a new compatible 'renesas,rzg2l-vsp2' to
> > > > handle these differences.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> > > > index 990e9c1dbc43..2696a4582251 100644
> > > > --- a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > > > +++ b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> 
> > > > +        clock-names:
> > > > +          items:
> > > > +            - const: du.0
> > >
> > > Similarly, I'm not sure this is a good name from the point of view
> > > of the VSP.
> >
> > OK, will use the name 'aclk', which is Main clock for this module
> > which is shared with LCDC. 'du.0' is not valid any more here as we are
> > using different CRTC implementation for RZ/G2LC.
> >
> > >
> > > > +            - const: pclk
> > > > +            - const: vclk
> > >
> > > I couldn't find those names in the documentation, where do they come
> > > from
> >
> > HW manual (page 312) mentions about LCDC_CLK_A, LCDC_CLK_P & LCDC_CLK_D.
> >
> > Detailed description is mentioned in Clock list document. Please see
> below.
> >
> >         LCDC_CLK_A      M0φ     PLL3    200     200             LCDC
> Main clock
> >         LCDC_CLK_P      ZTφ     PLL3    100     100             LCDC
> Register Access Clock
> >         LCDC_CLK_D      M3φ     SEL_PLL5_4      148.5~5.803     LCDC
> Video Clock
> >
> > > ? Could you maybe share a DT integration example ?
> >
> > Please see below,
> 
> >
> > +               du: display@0x10890000 {
> > +                       compatible = "renesas,du-r9a07g044l";
> > +                       reg = <0 0x10890000 0 0x10000>;
> > +                       interrupts = <GIC_SPI 152 IRQ_TYPE_LEVEL_HIGH>;
> > +                       clocks = <&cpg CPG_MOD R9A07G044_LCDC_CLK_A>,
> > +                                <&cpg CPG_MOD R9A07G044_LCDC_CLK_P>,
> > +                                <&cpg CPG_MOD R9A07G044_LCDC_CLK_D>;
> > +                       clock-names = "du.0", "pclk", "vclk";
> > +                       power-domains = <&cpg>;
> > +                       resets = <&cpg R9A07G044_LCDC_RESET_N>;
> > +                       reset-names = "du.0";
> > +                       renesas,vsps = <&vspd0 0>;
> 
> Given the DU driver is no longer shared, perhaps all occurrencies of "du"
> should be replaced by "lcdc"?

Ok to me. Will use "lcdc" and driver name will be "rzg2l-lcdc" instead of
"rzg2l-du". Will send a patch to MESA as well replacing 'rcar-du' with
'rzg2l-lcdc', once the lcdc driver hits mainline.

Cheers,
Biju

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

[Laurent Pinchart]

Hi Geert,

On Mon, Mar 14, 2022 at 10:01:14AM +0100, Geert Uytterhoeven wrote:
> On Mon, Mar 14, 2022 at 9:44 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > On Sat, Mar 12, 2022 at 08:42:03AM +0000, Biju Das wrote:
> > > > Document VSPD found in RZ/G2L and RZ/V2L family SoC's. VSPD block is
> > > > similar to VSP2-D found on R-Car SoC's, but it does not have a version
> > > > register and it has 3 clocks compared to 1 clock on vsp1 and vsp2.
> > > >
> > > > This patch introduces a new compatible 'renesas,rzg2l-vsp2' to handle
> > > > these differences.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> > > > index 990e9c1dbc43..2696a4582251 100644
> > > > --- a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > > > +++ b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> 
> > > > +        clock-names:
> > > > +          items:
> > > > +            - const: du.0
> > >
> > > Similarly, I'm not sure this is a good name from the point of view of the
> > > VSP.
> >
> > OK, will use the name 'aclk', which is Main clock for this module which is
> > shared with LCDC. 'du.0' is not valid any more here as we are using different
> > CRTC implementation for RZ/G2LC.
> >
> > > > +            - const: pclk
> > > > +            - const: vclk
> > >
> > > I couldn't find those names in the documentation, where do they come from
> >
> > HW manual (page 312) mentions about LCDC_CLK_A, LCDC_CLK_P & LCDC_CLK_D.
> >
> > Detailed description is mentioned in Clock list document. Please see below.
> >
> >         LCDC_CLK_A      M0φ     PLL3    200     200             LCDC  Main clock
> >         LCDC_CLK_P      ZTφ     PLL3    100     100             LCDC Register Access Clock
> >         LCDC_CLK_D      M3φ     SEL_PLL5_4      148.5~5.803     LCDC Video Clock
> >
> > > ? Could you maybe share a DT integration example ?
> >
> > Please see below,
> 
> >
> > +               du: display@0x10890000 {
> > +                       compatible = "renesas,du-r9a07g044l";
> > +                       reg = <0 0x10890000 0 0x10000>;
> > +                       interrupts = <GIC_SPI 152 IRQ_TYPE_LEVEL_HIGH>;
> > +                       clocks = <&cpg CPG_MOD R9A07G044_LCDC_CLK_A>,
> > +                                <&cpg CPG_MOD R9A07G044_LCDC_CLK_P>,
> > +                                <&cpg CPG_MOD R9A07G044_LCDC_CLK_D>;
> > +                       clock-names = "du.0", "pclk", "vclk";
> > +                       power-domains = <&cpg>;
> > +                       resets = <&cpg R9A07G044_LCDC_RESET_N>;
> > +                       reset-names = "du.0";
> > +                       renesas,vsps = <&vspd0 0>;
> 
> Given the DU driver is no longer shared, perhaps all occurrencies of "du"
> should be replaced by "lcdc"?

The LCDC is the combination of the FCPVD, the VSPD and the DU. The first
two are similar to the eponymous IP cores used on R-Car Gen3, while the
DU is a different beast, despite sharing the same name.

-- 
Regards,

Laurent Pinchart

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

[Geert Uytterhoeven]

Hi Laurent,

On Mon, Mar 14, 2022 at 1:13 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Mon, Mar 14, 2022 at 10:01:14AM +0100, Geert Uytterhoeven wrote:
> > On Mon, Mar 14, 2022 at 9:44 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > On Sat, Mar 12, 2022 at 08:42:03AM +0000, Biju Das wrote:
> > > > > Document VSPD found in RZ/G2L and RZ/V2L family SoC's. VSPD block is
> > > > > similar to VSP2-D found on R-Car SoC's, but it does not have a version
> > > > > register and it has 3 clocks compared to 1 clock on vsp1 and vsp2.
> > > > >
> > > > > This patch introduces a new compatible 'renesas,rzg2l-vsp2' to handle
> > > > > these differences.
> > > > >
> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >
> > > > > index 990e9c1dbc43..2696a4582251 100644
> > > > > --- a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> > > > > +++ b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> >
> > > > > +        clock-names:
> > > > > +          items:
> > > > > +            - const: du.0
> > > >
> > > > Similarly, I'm not sure this is a good name from the point of view of the
> > > > VSP.
> > >
> > > OK, will use the name 'aclk', which is Main clock for this module which is
> > > shared with LCDC. 'du.0' is not valid any more here as we are using different
> > > CRTC implementation for RZ/G2LC.
> > >
> > > > > +            - const: pclk
> > > > > +            - const: vclk
> > > >
> > > > I couldn't find those names in the documentation, where do they come from
> > >
> > > HW manual (page 312) mentions about LCDC_CLK_A, LCDC_CLK_P & LCDC_CLK_D.
> > >
> > > Detailed description is mentioned in Clock list document. Please see below.
> > >
> > >         LCDC_CLK_A      M0φ     PLL3    200     200             LCDC  Main clock
> > >         LCDC_CLK_P      ZTφ     PLL3    100     100             LCDC Register Access Clock
> > >         LCDC_CLK_D      M3φ     SEL_PLL5_4      148.5~5.803     LCDC Video Clock
> > >
> > > > ? Could you maybe share a DT integration example ?
> > >
> > > Please see below,
> >
> > >
> > > +               du: display@0x10890000 {
> > > +                       compatible = "renesas,du-r9a07g044l";
> > > +                       reg = <0 0x10890000 0 0x10000>;
> > > +                       interrupts = <GIC_SPI 152 IRQ_TYPE_LEVEL_HIGH>;
> > > +                       clocks = <&cpg CPG_MOD R9A07G044_LCDC_CLK_A>,
> > > +                                <&cpg CPG_MOD R9A07G044_LCDC_CLK_P>,
> > > +                                <&cpg CPG_MOD R9A07G044_LCDC_CLK_D>;
> > > +                       clock-names = "du.0", "pclk", "vclk";
> > > +                       power-domains = <&cpg>;
> > > +                       resets = <&cpg R9A07G044_LCDC_RESET_N>;
> > > +                       reset-names = "du.0";
> > > +                       renesas,vsps = <&vspd0 0>;
> >
> > Given the DU driver is no longer shared, perhaps all occurrencies of "du"
> > should be replaced by "lcdc"?
>
> The LCDC is the combination of the FCPVD, the VSPD and the DU. The first
> two are similar to the eponymous IP cores used on R-Car Gen3, while the
> DU is a different beast, despite sharing the same name.

OK, that's a good reason to keep the DU name.

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

[PATCH v5 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 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>
---
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/vsp1/vsp1.h     |  1 +
 drivers/media/platform/vsp1/vsp1_drv.c | 18 +++++++++++++++++-
 2 files changed, 18 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..699d7d985df4 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,16 @@ 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)
+		return ret;
+
+	ret = pm_runtime_resume_and_get(vsp1->dev);
+	if (ret < 0)
+		reset_control_assert(vsp1->rstc);
+
+	return ret;
 }
 
 /*
@@ -581,6 +591,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 +838,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 ctrl\n");
+
 	/* FCP (optional). */
 	fcp_node = of_parse_phandle(pdev->dev.of_node, "renesas,fcp", 0);
 	if (fcp_node) {
-- 
2.17.1

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

[Laurent Pinchart]

Hi Biju,

Thank you for the patch.

On Sat, Mar 12, 2022 at 08:42:04AM +0000, 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>
> ---
> 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/vsp1/vsp1.h     |  1 +
>  drivers/media/platform/vsp1/vsp1_drv.c | 18 +++++++++++++++++-
>  2 files changed, 18 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;

This is missing a forward declaration for struct reset_control.

>  	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..699d7d985df4 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,16 @@ 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)
> +		return ret;

I you don't mind, I'd prefer

	int ret;

	ret = reset_control_deassert(vsp1->rstc);
	if (ret < 0)
		return ret;

> +
> +	ret = pm_runtime_resume_and_get(vsp1->dev);
> +	if (ret < 0)
> +		reset_control_assert(vsp1->rstc);
> +
> +	return ret;
>  }
>  
>  /*
> @@ -581,6 +591,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 +838,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 ctrl\n");

s/ctrl/control/

With these small issues addressed,

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

> +
>  	/* FCP (optional). */
>  	fcp_node = of_parse_phandle(pdev->dev.of_node, "renesas,fcp", 0);
>  	if (fcp_node) {

-- 
Regards,

Laurent Pinchart

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

[Biju Das]

Hi Laurent,

Thanks for the feedback.

> Subject: Re: [PATCH v5 2/3] media: vsp1: Add support to deassert/assert
> reset line
> 
> Hi Biju,
> 
> Thank you for the patch.
> 
> On Sat, Mar 12, 2022 at 08:42:04AM +0000, 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>
> > ---
> > 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:
> >  *
> > ---
> >  drivers/media/platform/vsp1/vsp1.h     |  1 +
> >  drivers/media/platform/vsp1/vsp1_drv.c | 18 +++++++++++++++++-
> >  2 files changed, 18 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;
> 
> This is missing a forward declaration for struct reset_control.

OK, Will add forward declaration struct reset_control; in next version.

> 
> >  	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..699d7d985df4 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,16 @@ 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)
> > +		return ret;
> 
> I you don't mind, I'd prefer

OK for me, if there is no objections.

> 
> 	int ret;
> 
> 	ret = reset_control_deassert(vsp1->rstc);
> 	if (ret < 0)
> 		return ret;
> 
> > +
> > +	ret = pm_runtime_resume_and_get(vsp1->dev);
> > +	if (ret < 0)
> > +		reset_control_assert(vsp1->rstc);
> > +
> > +	return ret;
> >  }
> >
> >  /*
> > @@ -581,6 +591,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 +838,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 ctrl\n");
> 
> s/ctrl/control/
> 
> With these small issues addressed,

OK for me.

Regards,
Biju

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +
> >  	/* FCP (optional). */
> >  	fcp_node = of_parse_phandle(pdev->dev.of_node, "renesas,fcp", 0);
> >  	if (fcp_node) {
> 
> --
> Regards,
> 
> Laurent Pinchart

[PATCH v5 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,rzg2l-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>
---
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/vsp1/vsp1_drv.c  | 44 +++++++++++++++++++------
 drivers/media/platform/vsp1/vsp1_lif.c  | 16 +++++----
 drivers/media/platform/vsp1/vsp1_regs.h |  4 +++
 3 files changed, 48 insertions(+), 16 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
index 699d7d985df4..4eef6d525eda 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -811,11 +811,37 @@ static const struct vsp1_device_info vsp1_device_infos[] = {
 	},
 };
 
+static const struct vsp1_device_info rzg2l_vsp2_device_info = {
+		.version = VI6_IP_VERSION_MODEL_VSPD_RZG2L,
+		.model = "VSP2-D",
+		.gen = 3,
+		.features = VSP1_HAS_BRS | VSP1_HAS_WPF_VFLIP | VSP1_HAS_EXT_DL,
+		.lif_count = 1,
+		.rpf_count = 2,
+		.wpf_count = 1,
+};
+
+static const struct vsp1_device_info *vsp1_lookup(struct vsp1_device *vsp1,
+						  u32 version)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
+		if ((version & VI6_IP_VERSION_MODEL_MASK) ==
+		    vsp1_device_infos[i].version) {
+			vsp1->info = &vsp1_device_infos[i];
+			break;
+		}
+	}
+
+	return vsp1->info;
+}
+
 static int vsp1_probe(struct platform_device *pdev)
 {
 	struct vsp1_device *vsp1;
 	struct device_node *fcp_node;
-	unsigned int i;
+	u32 version;
 	int ret;
 	int irq;
 
@@ -871,24 +897,21 @@ 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 = of_device_get_match_data(&pdev->dev);
+	if (!vsp1->info) {
+		version = vsp1_read(vsp1, VI6_IP_VERSION);
+		vsp1->info = vsp1_lookup(vsp1, version);
 	}
 
 	if (!vsp1->info) {
 		dev_err(&pdev->dev, "unsupported IP version 0x%08x\n",
-			vsp1->version);
+			version);
 		vsp1_device_put(vsp1);
 		ret = -ENXIO;
 		goto done;
 	}
 
+	vsp1->version = vsp1->info->version;
 	dev_dbg(&pdev->dev, "IP version 0x%08x\n", vsp1->version);
 
 	/*
@@ -940,6 +963,7 @@ static int vsp1_remove(struct platform_device *pdev)
 static const struct of_device_id vsp1_of_match[] = {
 	{ .compatible = "renesas,vsp1" },
 	{ .compatible = "renesas,vsp2" },
+	{ .compatible = "renesas,rzg2l-vsp2", .data = &rzg2l_vsp2_device_info },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, vsp1_of_match);
diff --git a/drivers/media/platform/vsp1/vsp1_lif.c b/drivers/media/platform/vsp1/vsp1_lif.c
index 6a6857ac9327..35abed29f269 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;
@@ -130,16 +131,19 @@ static void lif_configure_stream(struct vsp1_entity *entity,
 			VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN);
 
 	/*
-	 * On R-Car V3M the LIF0 buffer attribute register has to be set to a
-	 * non-default value to guarantee proper operation (otherwise artifacts
-	 * may appear on the output). The value required by the manual is not
-	 * explained but is likely a buffer size or threshold.
+	 * On R-Car V3M and RZ/G2L the LIF0 buffer attribute register has to be
+	 * set to a non-default value to guarantee proper operation (otherwise
+	 * artifacts may appear on the output). The value required by the
+	 * manual is not explained but is likely a buffer size or threshold.
 	 */
-	if ((entity->vsp1->version & VI6_IP_VERSION_MASK) ==
-	    (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M))
+	switch (entity->vsp1->version & VI6_IP_VERSION_MASK) {
+	case (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M):
+	case (VI6_IP_VERSION_MODEL_VSPD_RZG2L | VI6_IP_VERSION_SOC_G2L):
 		vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
 			       VI6_LIF_LBA_LBA0 |
 			       (1536 << VI6_LIF_LBA_LBA1_SHIFT));
+		break;
+	}
 }
 
 static const struct vsp1_entity_operations lif_entity_ops = {
diff --git a/drivers/media/platform/vsp1/vsp1_regs.h b/drivers/media/platform/vsp1/vsp1_regs.h
index fae7286eb01e..3963119eecc5 100644
--- a/drivers/media/platform/vsp1/vsp1_regs.h
+++ b/drivers/media/platform/vsp1/vsp1_regs.h
@@ -767,8 +767,12 @@
 #define VI6_IP_VERSION_MODEL_VSPDL_GEN3	(0x19 << 8)
 #define VI6_IP_VERSION_MODEL_VSPBS_GEN3	(0x1a << 8)
 #define VI6_IP_VERSION_MODEL_VSPD_V3U	(0x1c << 8)
+/* RZ/G2L SoC's have no version register, So using last ID for the version */
+#define VI6_IP_VERSION_MODEL_VSPD_RZG2L	(0xff << 8)
 
 #define VI6_IP_VERSION_SOC_MASK		(0xff << 0)
+/* RZ/G2L SoC's have no version register, So use '0' for SoC Identification */
+#define VI6_IP_VERSION_SOC_G2L		(0x00 << 0)
 #define VI6_IP_VERSION_SOC_H2		(0x01 << 0)
 #define VI6_IP_VERSION_SOC_V2H		(0x01 << 0)
 #define VI6_IP_VERSION_SOC_V3M		(0x01 << 0)
-- 
2.17.1

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

[Laurent Pinchart]

Hi Biju,

Thank you for the patch.

On Sat, Mar 12, 2022 at 08:42:05AM +0000, Biju Das wrote:
> The RZ/G2L VSPD provides a single VSPD instance. It has the following
> sub modules MAU, CTU, RPF, DPR, LUT, BRS, WPF and LIF.
> 
> The VSPD block on RZ/G2L does not have a version register, so added a
> new compatible string "renesas,rzg2l-vsp2" with a data pointer containing
> the info structure. Also the reset line is shared with the DU module.
> 
> 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>
> ---
> 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/vsp1/vsp1_drv.c  | 44 +++++++++++++++++++------
>  drivers/media/platform/vsp1/vsp1_lif.c  | 16 +++++----
>  drivers/media/platform/vsp1/vsp1_regs.h |  4 +++
>  3 files changed, 48 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
> index 699d7d985df4..4eef6d525eda 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -811,11 +811,37 @@ static const struct vsp1_device_info vsp1_device_infos[] = {
>  	},
>  };
>  
> +static const struct vsp1_device_info rzg2l_vsp2_device_info = {
> +		.version = VI6_IP_VERSION_MODEL_VSPD_RZG2L,
> +		.model = "VSP2-D",
> +		.gen = 3,
> +		.features = VSP1_HAS_BRS | VSP1_HAS_WPF_VFLIP | VSP1_HAS_EXT_DL,
> +		.lif_count = 1,
> +		.rpf_count = 2,
> +		.wpf_count = 1,

One tab is enough for indentation.

> +};
> +
> +static const struct vsp1_device_info *vsp1_lookup(struct vsp1_device *vsp1,
> +						  u32 version)

Let's call this vsp1_lookup_info(), just "lookup" is a bit vague.

> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
> +		if ((version & VI6_IP_VERSION_MODEL_MASK) ==
> +		    vsp1_device_infos[i].version) {
> +			vsp1->info = &vsp1_device_infos[i];
> +			break;
> +		}
> +	}
> +
> +	return vsp1->info;

Could we move all the info logic in this function ? Something like this:

	const struct vsp1_device_info *info;
	unsigned int i;
	u32 version;

	/*
	 * 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)
		return info;

	version = vsp1_read(vsp1, VI6_IP_VERSION);

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

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

	dev_err(vsp1->dev, "unsupported IP version 0x%08x\n", version);

	return NULL;

> +}
> +
>  static int vsp1_probe(struct platform_device *pdev)
>  {
>  	struct vsp1_device *vsp1;
>  	struct device_node *fcp_node;
> -	unsigned int i;
> +	u32 version;
>  	int ret;
>  	int irq;
>  
> @@ -871,24 +897,21 @@ 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 = of_device_get_match_data(&pdev->dev);
> +	if (!vsp1->info) {
> +		version = vsp1_read(vsp1, VI6_IP_VERSION);
> +		vsp1->info = vsp1_lookup(vsp1, version);
>  	}
>  
>  	if (!vsp1->info) {
>  		dev_err(&pdev->dev, "unsupported IP version 0x%08x\n",
> -			vsp1->version);
> +			version);
>  		vsp1_device_put(vsp1);
>  		ret = -ENXIO;
>  		goto done;
>  	}
>  
> +	vsp1->version = vsp1->info->version;

And here you would have

	vsp1->info = vsp1_lookup_info(vsp1);
	if (!vsp1->info) {
		ret = -ENXIO;
		goto done;
	}

	vsp1->version = vsp1->info->version;

A subsequent patch could even drop the version field from vsp1_device
and use vsp1->info->version instead of vsp1->version.

There's however a small issue. The version number is exposed to
userspace. Currently it's a full 32-bit value with the 16 MSBs being
0x0101 for all VSP versions, and the 16 LSBs identifying the VSP model
and SoC. With this patch, the version number is retrieved from the info
structure, and the 16 MSBs will thus be 0x0000. This may cause issues in
userspace.

One option would be to set the 16 MSBs in vsp1_device_info.version
(using a new macro added to vsp1_regs.h, I'm not sure how to call it as
it's not clear what 0x0101 represents, if it's meant to identify the
fact that the device is a VSP, maybe VI6_IP_VERSION_VSP ?).

>  	dev_dbg(&pdev->dev, "IP version 0x%08x\n", vsp1->version);
>  
>  	/*
> @@ -940,6 +963,7 @@ static int vsp1_remove(struct platform_device *pdev)
>  static const struct of_device_id vsp1_of_match[] = {
>  	{ .compatible = "renesas,vsp1" },
>  	{ .compatible = "renesas,vsp2" },
> +	{ .compatible = "renesas,rzg2l-vsp2", .data = &rzg2l_vsp2_device_info },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, vsp1_of_match);
> diff --git a/drivers/media/platform/vsp1/vsp1_lif.c b/drivers/media/platform/vsp1/vsp1_lif.c
> index 6a6857ac9327..35abed29f269 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;
> @@ -130,16 +131,19 @@ static void lif_configure_stream(struct vsp1_entity *entity,
>  			VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN);
>  
>  	/*
> -	 * On R-Car V3M the LIF0 buffer attribute register has to be set to a
> -	 * non-default value to guarantee proper operation (otherwise artifacts
> -	 * may appear on the output). The value required by the manual is not
> -	 * explained but is likely a buffer size or threshold.
> +	 * On R-Car V3M and RZ/G2L the LIF0 buffer attribute register has to be
> +	 * set to a non-default value to guarantee proper operation (otherwise
> +	 * artifacts may appear on the output). The value required by the
> +	 * manual is not explained but is likely a buffer size or threshold.
>  	 */
> -	if ((entity->vsp1->version & VI6_IP_VERSION_MASK) ==
> -	    (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M))
> +	switch (entity->vsp1->version & VI6_IP_VERSION_MASK) {
> +	case (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M):
> +	case (VI6_IP_VERSION_MODEL_VSPD_RZG2L | VI6_IP_VERSION_SOC_G2L):
>  		vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
>  			       VI6_LIF_LBA_LBA0 |
>  			       (1536 << VI6_LIF_LBA_LBA1_SHIFT));
> +		break;
> +	}
>  }
>  
>  static const struct vsp1_entity_operations lif_entity_ops = {
> diff --git a/drivers/media/platform/vsp1/vsp1_regs.h b/drivers/media/platform/vsp1/vsp1_regs.h
> index fae7286eb01e..3963119eecc5 100644
> --- a/drivers/media/platform/vsp1/vsp1_regs.h
> +++ b/drivers/media/platform/vsp1/vsp1_regs.h
> @@ -767,8 +767,12 @@
>  #define VI6_IP_VERSION_MODEL_VSPDL_GEN3	(0x19 << 8)
>  #define VI6_IP_VERSION_MODEL_VSPBS_GEN3	(0x1a << 8)
>  #define VI6_IP_VERSION_MODEL_VSPD_V3U	(0x1c << 8)
> +/* RZ/G2L SoC's have no version register, So using last ID for the version */
> +#define VI6_IP_VERSION_MODEL_VSPD_RZG2L	(0xff << 8)
>  
>  #define VI6_IP_VERSION_SOC_MASK		(0xff << 0)
> +/* RZ/G2L SoC's have no version register, So use '0' for SoC Identification */
> +#define VI6_IP_VERSION_SOC_G2L		(0x00 << 0)

Hmmmm... I wonder how we can make sure here that no future SoCs will use
0xff or 0x00. Especially from a userspace point of view, The resulting
version number will be 0x0000ff00, I'm not sure it will let us have a
nice versioning scheme if we have to extend this to new SoCs that have
different VSPs also lacking a version register.

I wonder if we could possibly use the 16 MSBs to differentiate between
real hardware versions and synthetic versions. For instance, we could
set the MSBs to 0xfffe (unlikely to be used by real hardware), and then
we could allocate version numbers for the 16 LSBs freely, starting
numbering at 0x01 (or maybe a higher value, such as 0x80 ?) for the
model and SoC identifiers.

What does everybody think ?

>  #define VI6_IP_VERSION_SOC_H2		(0x01 << 0)
>  #define VI6_IP_VERSION_SOC_V2H		(0x01 << 0)
>  #define VI6_IP_VERSION_SOC_V3M		(0x01 << 0)
> 

-- 
Regards,

Laurent Pinchart

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

[Biju Das]

Hi Laurent,

Thanks for the feedback.

> Subject: Re: [PATCH v5 3/3] media: vsp1: Add support for RZ/G2L VSPD
> 
> Hi Biju,
> 
> Thank you for the patch.
> 
> On Sat, Mar 12, 2022 at 08:42:05AM +0000, Biju Das wrote:
> > The RZ/G2L VSPD provides a single VSPD instance. It has the following
> > sub modules MAU, CTU, RPF, DPR, LUT, BRS, WPF and LIF.
> >
> > The VSPD block on RZ/G2L does not have a version register, so added a
> > new compatible string "renesas,rzg2l-vsp2" with a data pointer
> > containing the info structure. Also the reset line is shared with the DU
> module.
> >
> > 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>
> > ---
> > v4->v5:
> >  * Fixed typo VI6_IP_VERSION_MODEL_MASK->VI6_IP_VERSION_MASK
> >  * To be consistent with other SoC's, introduced VI6_IP_VERSION_SOC_G2L
> >    for RZ/G2L SoC's.
> > v3->v4:
> >  * Added Rb tag from Geert
> >  * Add switch() for LIF0 buffer attribute handling for RZ/G2L and V3M
> > v2->v3:
> >  * Fixed version comparison in vsp1_lookup()
> > v1->v2:
> >  * Changed the compatible from vsp2-rzg2l->rzg2l-vsp2
> >  * Added standalone device info for rzg2l-vsp2.
> >  * Added vsp1_lookup helper function.
> >  * Updated comments for LIF0 buffer attribute register
> >  * Used last ID for rzg2l-vsp2.
> > RFC->v1:
> >  * Used data pointer containing info structure to retrieve version
> > information
> > RFC:
> >  *
> > ---
> >  drivers/media/platform/vsp1/vsp1_drv.c  | 44
> > +++++++++++++++++++------  drivers/media/platform/vsp1/vsp1_lif.c  |
> > 16 +++++----  drivers/media/platform/vsp1/vsp1_regs.h |  4 +++
> >  3 files changed, 48 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
> > b/drivers/media/platform/vsp1/vsp1_drv.c
> > index 699d7d985df4..4eef6d525eda 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drv.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> > @@ -811,11 +811,37 @@ static const struct vsp1_device_info
> vsp1_device_infos[] = {
> >  	},
> >  };
> >
> > +static const struct vsp1_device_info rzg2l_vsp2_device_info = {
> > +		.version = VI6_IP_VERSION_MODEL_VSPD_RZG2L,
> > +		.model = "VSP2-D",
> > +		.gen = 3,
> > +		.features = VSP1_HAS_BRS | VSP1_HAS_WPF_VFLIP |
> VSP1_HAS_EXT_DL,
> > +		.lif_count = 1,
> > +		.rpf_count = 2,
> > +		.wpf_count = 1,
> 
> One tab is enough for indentation.

Agreed. Will fix this.

> 
> > +};
> > +
> > +static const struct vsp1_device_info *vsp1_lookup(struct vsp1_device
> *vsp1,
> > +						  u32 version)
> 
> Let's call this vsp1_lookup_info(), just "lookup" is a bit vague.

OK.

> 
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
> > +		if ((version & VI6_IP_VERSION_MODEL_MASK) ==
> > +		    vsp1_device_infos[i].version) {
> > +			vsp1->info = &vsp1_device_infos[i];
> > +			break;
> > +		}
> > +	}
> > +
> > +	return vsp1->info;
> 
> Could we move all the info logic in this function ? Something like this:

OK for me, If there is no objections.

What about using vsp1->version instead of local variable version,
So that in case of synthetic version, we can override MSB's with
0xFFFE (Please see below)

> 
> 	const struct vsp1_device_info *info;
> 	unsigned int i;
> 	u32 version;

Here Remove the local variable version and instead use vsp1->version.
> 
> 	/*
> 	 * 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)
> 		return info;
> 
> 	version = vsp1_read(vsp1, VI6_IP_VERSION);
	

vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);

> 
> 	for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
> 		info = &vsp1_device_infos[i];
> 
> 		if ((version & VI6_IP_VERSION_MODEL_MASK) == info->version);
> 			return info;
> 	}
> 
> 	dev_err(vsp1->dev, "unsupported IP version 0x%08x\n", version);
> 
> 	return NULL;
> 
> > +}
> > +
> >  static int vsp1_probe(struct platform_device *pdev)  {
> >  	struct vsp1_device *vsp1;
> >  	struct device_node *fcp_node;
> > -	unsigned int i;
> > +	u32 version;
> >  	int ret;
> >  	int irq;
> >
> > @@ -871,24 +897,21 @@ 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 = of_device_get_match_data(&pdev->dev);
> > +	if (!vsp1->info) {
> > +		version = vsp1_read(vsp1, VI6_IP_VERSION);
> > +		vsp1->info = vsp1_lookup(vsp1, version);
> >  	}
> >
> >  	if (!vsp1->info) {
> >  		dev_err(&pdev->dev, "unsupported IP version 0x%08x\n",
> > -			vsp1->version);
> > +			version);
> >  		vsp1_device_put(vsp1);
> >  		ret = -ENXIO;
> >  		goto done;
> >  	}
> >
> > +	vsp1->version = vsp1->info->version;
> 
> And here you would have
> 
> 	vsp1->info = vsp1_lookup_info(vsp1);
> 	if (!vsp1->info) {
> 		ret = -ENXIO;
> 		goto done;
> 	}
> 
> 	vsp1->version = vsp1->info->version;

Here some thing like
(vsp1->version & 0x0101)--> real hw version
Else
Synthetic version {
vsp1->version = (0xFFFE0000)| vsp1->info->version | VI6_IP_VERSION_SOC_RZG2L;
}

so that it won't break user space as mentioned below.

> 
> A subsequent patch could even drop the version field from vsp1_device and
> use vsp1->info->version instead of vsp1->version.

May be it is not needed ??

> 
> There's however a small issue. The version number is exposed to userspace.
> Currently it's a full 32-bit value with the 16 MSBs being
> 0x0101 for all VSP versions, and the 16 LSBs identifying the VSP model and
> SoC. With this patch, the version number is retrieved from the info
> structure, and the 16 MSBs will thus be 0x0000. This may cause issues in
> userspace.
> 
> One option would be to set the 16 MSBs in vsp1_device_info.version (using
> a new macro added to vsp1_regs.h, I'm not sure how to call it as it's not
> clear what 0x0101 represents, if it's meant to identify the fact that the
> device is a VSP, maybe VI6_IP_VERSION_VSP ?).

HW document doesn't mention about what 0x0101 represents for MSBs? 
What about VI6_IP_VERSION_VSP_HW_MSB to differentiate HW version.

> 
> >  	dev_dbg(&pdev->dev, "IP version 0x%08x\n", vsp1->version);
> >
> >  	/*
> > @@ -940,6 +963,7 @@ static int vsp1_remove(struct platform_device
> > *pdev)  static const struct of_device_id vsp1_of_match[] = {
> >  	{ .compatible = "renesas,vsp1" },
> >  	{ .compatible = "renesas,vsp2" },
> > +	{ .compatible = "renesas,rzg2l-vsp2", .data =
> > +&rzg2l_vsp2_device_info },
> >  	{ },
> >  };
> >  MODULE_DEVICE_TABLE(of, vsp1_of_match); diff --git
> > a/drivers/media/platform/vsp1/vsp1_lif.c
> > b/drivers/media/platform/vsp1/vsp1_lif.c
> > index 6a6857ac9327..35abed29f269 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;
> > @@ -130,16 +131,19 @@ static void lif_configure_stream(struct
> vsp1_entity *entity,
> >  			VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN);
> >
> >  	/*
> > -	 * On R-Car V3M the LIF0 buffer attribute register has to be set to
> a
> > -	 * non-default value to guarantee proper operation (otherwise
> artifacts
> > -	 * may appear on the output). The value required by the manual is
> not
> > -	 * explained but is likely a buffer size or threshold.
> > +	 * On R-Car V3M and RZ/G2L the LIF0 buffer attribute register has to
> be
> > +	 * set to a non-default value to guarantee proper operation
> (otherwise
> > +	 * artifacts may appear on the output). The value required by the
> > +	 * manual is not explained but is likely a buffer size or threshold.
> >  	 */
> > -	if ((entity->vsp1->version & VI6_IP_VERSION_MASK) ==
> > -	    (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M))
> > +	switch (entity->vsp1->version & VI6_IP_VERSION_MASK) {
> > +	case (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M):
> > +	case (VI6_IP_VERSION_MODEL_VSPD_RZG2L | VI6_IP_VERSION_SOC_G2L):
> >  		vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
> >  			       VI6_LIF_LBA_LBA0 |
> >  			       (1536 << VI6_LIF_LBA_LBA1_SHIFT));
> > +		break;
> > +	}
> >  }
> >
> >  static const struct vsp1_entity_operations lif_entity_ops = { diff
> > --git a/drivers/media/platform/vsp1/vsp1_regs.h
> > b/drivers/media/platform/vsp1/vsp1_regs.h
> > index fae7286eb01e..3963119eecc5 100644
> > --- a/drivers/media/platform/vsp1/vsp1_regs.h
> > +++ b/drivers/media/platform/vsp1/vsp1_regs.h
> > @@ -767,8 +767,12 @@
> >  #define VI6_IP_VERSION_MODEL_VSPDL_GEN3	(0x19 << 8)
> >  #define VI6_IP_VERSION_MODEL_VSPBS_GEN3	(0x1a << 8)
> >  #define VI6_IP_VERSION_MODEL_VSPD_V3U	(0x1c << 8)
> > +/* RZ/G2L SoC's have no version register, So using last ID for the
> version */
> > +#define VI6_IP_VERSION_MODEL_VSPD_RZG2L	(0xff << 8)
> >
> >  #define VI6_IP_VERSION_SOC_MASK		(0xff << 0)
> > +/* RZ/G2L SoC's have no version register, So use '0' for SoC
> Identification */
> > +#define VI6_IP_VERSION_SOC_G2L		(0x00 << 0)
> 
> Hmmmm... I wonder how we can make sure here that no future SoCs will use
> 0xff or 0x00. Especially from a userspace point of view, The resulting
> version number will be 0x0000ff00, I'm not sure it will let us have a nice
> versioning scheme if we have to extend this to new SoCs that have
> different VSPs also lacking a version register.
> 
> I wonder if we could possibly use the 16 MSBs to differentiate between
> real hardware versions and synthetic versions. For instance, we could set
> the MSBs to 0xfffe (unlikely to be used by real hardware), and then we
> could allocate version numbers for the 16 LSBs freely, starting numbering
> at 0x01 (or maybe a higher value, such as 0x80 ?) for the model and SoC
> identifiers.
> 
> What does everybody think ?

I am ok for synthetic version of "0xfffe8000" for RZ/G2L, if there is 
no objections and add 0xFFFE in the current 

VI6_IP_VERSION_VSP_SW_MSB		(0xFFFE << 16)
VI6_IP_VERSION_MODEL_VSPD_RZG2L	(0x80 << 8)
VI6_IP_VERSION_SOC_RZG2L		(0x00 << 0)

We could directly use "entity->vsp1->version" instead of 
'entity->vsp1->version & VI6_IP_VERSION_MASK'
for the switch() ('switch (entity->vsp1->version & VI6_IP_VERSION_MASK)') as

switch(entity->vsp1->version) has already differentiation between real and 
synthetic version.

VI6_IP_VERSION_V3M ((vsp1->version & VI6_IP_VERSION_VSP_HW_MSB) | VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M)
VI6_IP_VERSION_RZG2L ((vsp1->version & VI6_IP_VERSION_VSP_SW_MSB) | IP_VERSION_MODEL_VSPD_RZG2L | VI6_IP_VERSION_SOC_RZG2L)

Cheers,
Biju


> 
> >  #define VI6_IP_VERSION_SOC_H2		(0x01 << 0)
> >  #define VI6_IP_VERSION_SOC_V2H		(0x01 << 0)
> >  #define VI6_IP_VERSION_SOC_V3M		(0x01 << 0)
> >
> 
> --
> Regards,
> 
> Laurent Pinchart