[PATCH v3 0/2] rcar-vin: Add support for R-Car V4M

[PATCH v3 0/2] rcar-vin: Add support for R-Car V4M

[Niklas Söderlund]

Hello,

This small series adds bindings and support to rcar-vin for R-Car V4M.

The two patches where previously posted separately as v1, but are now
collected in a single series.

See individual patches for changes since previous version.

Niklas Söderlund (2):
  dt-bindings: media: renesas,vin: Add binding for V4M
  media: rcar-vin: Add support for R-Car V4M

 Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 +
 drivers/media/platform/renesas/rcar-vin/rcar-core.c      | 8 ++++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.45.2

[PATCH v3 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Niklas Söderlund]

Document support for the VIN module in the Renesas V4M (r8a779h0) SoC.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
index 5539d0f8e74d..168cb02f8abe 100644
--- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
+++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
@@ -54,6 +54,7 @@ properties:
               - renesas,vin-r8a77995 # R-Car D3
               - renesas,vin-r8a779a0 # R-Car V3U
               - renesas,vin-r8a779g0 # R-Car V4H
+              - renesas,vin-r8a779h0 # R-Car V4M
 
   reg:
     maxItems: 1
-- 
2.45.2

Re: [PATCH v3 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1194 bytes --]

On Wed, Jun 19, 2024 at 05:35:58PM +0200, Niklas Söderlund wrote:
> Document support for the VIN module in the Renesas V4M (r8a779h0) SoC.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Didn't we just have a conversation about this, yet nothing has changed?
NAK. Either you need a fallback or to explain why a fallback is not
suitable _in this patch_.

Thanks,
Conor.

> ---
>  Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> index 5539d0f8e74d..168cb02f8abe 100644
> --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> @@ -54,6 +54,7 @@ properties:
>                - renesas,vin-r8a77995 # R-Car D3
>                - renesas,vin-r8a779a0 # R-Car V3U
>                - renesas,vin-r8a779g0 # R-Car V4H
> +              - renesas,vin-r8a779h0 # R-Car V4M
>  
>    reg:
>      maxItems: 1
> -- 
> 2.45.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Niklas Söderlund]

Hi Conor,

On 2024-06-19 18:33:37 +0100, Conor Dooley wrote:
> On Wed, Jun 19, 2024 at 05:35:58PM +0200, Niklas Söderlund wrote:
> > Document support for the VIN module in the Renesas V4M (r8a779h0) SoC.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Didn't we just have a conversation about this, yet nothing has changed?
> NAK. Either you need a fallback or to explain why a fallback is not
> suitable _in this patch_.

Sorry, I'm confused from the conclusion of our conversation in v2. I did 
add an explanation to why not fallback is used, but I added it to patch 
2/2 which adds the compatible to the driver.

It was my understanding that a SoC specific compatible was needed in 
either case so, at lest to me, made more sens to explain why in the 
driver patch the reason go into detail about the register differences 
between the two. Sorry if I misunderstood. I can add the same 
explanation to both patches, would this help explain why only a SoC 
specific value is added?

  The datasheet for the two SoCs have small nuances around the Pre-Clip
  registers ELPrC and EPPrC in three use-cases, interlaced images,
  embedded data and RAW8 input. On V4H the values written to the registers
  are based on odd numbers while on V4M they are even numbers, based on
  the input image size.

  No board that uses these SoCs which also have the external peripherals
  to test these nuances exists. Most likely this is an issue in the
  datasheet, but to make this easy to address in the future do not add a
  common Gen4 fallback compatible. Instead uses SoC specific compatibles
  for both SoCs. This is what was done for Gen3 SoCs, which also had
  similar nuances in the register documentation.

> 
> Thanks,
> Conor.
> 
> > ---
> >  Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > index 5539d0f8e74d..168cb02f8abe 100644
> > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > @@ -54,6 +54,7 @@ properties:
> >                - renesas,vin-r8a77995 # R-Car D3
> >                - renesas,vin-r8a779a0 # R-Car V3U
> >                - renesas,vin-r8a779g0 # R-Car V4H
> > +              - renesas,vin-r8a779h0 # R-Car V4M
> >  
> >    reg:
> >      maxItems: 1
> > -- 
> > 2.45.2
> > 



-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v3 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Niklas Söderlund]

Hello again.

On 2024-06-19 20:56:11 +0200, Niklas Söderlund wrote:
> Hi Conor,
> 
> On 2024-06-19 18:33:37 +0100, Conor Dooley wrote:
> > On Wed, Jun 19, 2024 at 05:35:58PM +0200, Niklas Söderlund wrote:
> > > Document support for the VIN module in the Renesas V4M (r8a779h0) SoC.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > 
> > Didn't we just have a conversation about this, yet nothing has changed?
> > NAK. Either you need a fallback or to explain why a fallback is not
> > suitable _in this patch_.
> 
> Sorry, I'm confused from the conclusion of our conversation in v2. I did 
> add an explanation to why not fallback is used, but I added it to patch 
> 2/2 which adds the compatible to the driver.
> 
> It was my understanding that a SoC specific compatible was needed in 
> either case so, at lest to me, made more sens to explain why in the 
> driver patch the reason go into detail about the register differences 
> between the two. Sorry if I misunderstood. I can add the same 
> explanation to both patches, would this help explain why only a SoC 
> specific value is added?
> 
>   The datasheet for the two SoCs have small nuances around the Pre-Clip
>   registers ELPrC and EPPrC in three use-cases, interlaced images,
>   embedded data and RAW8 input. On V4H the values written to the registers
>   are based on odd numbers while on V4M they are even numbers, based on
>   the input image size.
> 
>   No board that uses these SoCs which also have the external peripherals
>   to test these nuances exists. Most likely this is an issue in the
>   datasheet, but to make this easy to address in the future do not add a
>   common Gen4 fallback compatible. Instead uses SoC specific compatibles
>   for both SoCs. This is what was done for Gen3 SoCs, which also had
>   similar nuances in the register documentation.

After have read thru v1 and v2 comments a few more times I think I might 
have spotted what I got wrong. If so I apologies for wasting your time 
reviewing this. I'm really trying to understand what I got wrong and 
address the review feedback.

Is what you are asking for with a fallback something like this?

--- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
+++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
@@ -53,7 +53,11 @@ properties:
               - renesas,vin-r8a77990 # R-Car E3
               - renesas,vin-r8a77995 # R-Car D3
               - renesas,vin-r8a779a0 # R-Car V3U
+      - items:
+          - enum:
               - renesas,vin-r8a779g0 # R-Car V4H
+              - renesas,vin-r8a779h0 # R-Car V4M
+          - const: renesas,rcar-gen4-vin # Generic R-Car Gen4

If so I can see that working as I could still fix any issues that come 
from differences between V4H and V4M if needed. If so do you think it 
best to add this in two different patches? One to add the 
renesas,rcar-gen4-vin fallback (which will also need DTS updates to fix 
warnings from exciting users of V4H not listing the gen4 fallback) and 
one to add V4M?

Apologies again for the confusion.

> 
> > 
> > Thanks,
> > Conor.
> > 
> > > ---
> > >  Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > index 5539d0f8e74d..168cb02f8abe 100644
> > > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > @@ -54,6 +54,7 @@ properties:
> > >                - renesas,vin-r8a77995 # R-Car D3
> > >                - renesas,vin-r8a779a0 # R-Car V3U
> > >                - renesas,vin-r8a779g0 # R-Car V4H
> > > +              - renesas,vin-r8a779h0 # R-Car V4M
> > >  
> > >    reg:
> > >      maxItems: 1
> > > -- 
> > > 2.45.2
> > > 
> 
> 
> 
> -- 
> Kind Regards,
> Niklas Söderlund

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v3 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4481 bytes --]

On Wed, Jun 19, 2024 at 10:43:21PM +0200, Niklas Söderlund wrote:
> Hello again.
> 
> On 2024-06-19 20:56:11 +0200, Niklas Söderlund wrote:
> > Hi Conor,
> > 
> > On 2024-06-19 18:33:37 +0100, Conor Dooley wrote:
> > > On Wed, Jun 19, 2024 at 05:35:58PM +0200, Niklas Söderlund wrote:
> > > > Document support for the VIN module in the Renesas V4M (r8a779h0) SoC.
> > > > 
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > 
> > > Didn't we just have a conversation about this, yet nothing has changed?
> > > NAK. Either you need a fallback or to explain why a fallback is not
> > > suitable _in this patch_.
> > 
> > Sorry, I'm confused from the conclusion of our conversation in v2. I did 
> > add an explanation to why not fallback is used, but I added it to patch 
> > 2/2 which adds the compatible to the driver.

If you're unsure at all just ask, better that than send a new version.

> > 
> > It was my understanding that a SoC specific compatible was needed in 
> > either case so, at lest to me, made more sens to explain why in the 
> > driver patch the reason go into detail about the register differences 
> > between the two. Sorry if I misunderstood. I can add the same 
> > explanation to both patches, would this help explain why only a SoC 
> > specific value is added?
> > 
> >   The datasheet for the two SoCs have small nuances around the Pre-Clip
> >   registers ELPrC and EPPrC in three use-cases, interlaced images,
> >   embedded data and RAW8 input. On V4H the values written to the registers
> >   are based on odd numbers while on V4M they are even numbers, based on
> >   the input image size.
> > 
> >   No board that uses these SoCs which also have the external peripherals
> >   to test these nuances exists. Most likely this is an issue in the
> >   datasheet, but to make this easy to address in the future do not add a
> >   common Gen4 fallback compatible. Instead uses SoC specific compatibles
> >   for both SoCs. This is what was done for Gen3 SoCs, which also had
> >   similar nuances in the register documentation.
> 
> After have read thru v1 and v2 comments a few more times I think I might 
> have spotted what I got wrong. If so I apologies for wasting your time 
> reviewing this. I'm really trying to understand what I got wrong and 
> address the review feedback.
> 
> Is what you are asking for with a fallback something like this?
> 
> --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> @@ -53,7 +53,11 @@ properties:
>                - renesas,vin-r8a77990 # R-Car E3
>                - renesas,vin-r8a77995 # R-Car D3
>                - renesas,vin-r8a779a0 # R-Car V3U
> +      - items:
> +          - enum:
>                - renesas,vin-r8a779g0 # R-Car V4H
> +              - renesas,vin-r8a779h0 # R-Car V4M
> +          - const: renesas,rcar-gen4-vin # Generic R-Car Gen4
> 
> If so I can see that working as I could still fix any issues that come 
> from differences between V4H and V4M if needed. If so do you think it 
> best to add this in two different patches? One to add the 
> renesas,rcar-gen4-vin fallback (which will also need DTS updates to fix 
> warnings from exciting users of V4H not listing the gen4 fallback) and 
> one to add V4M?


I would just do:
diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
index 5539d0f8e74d..22bbad42fc03 100644
--- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
+++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
@@ -54,6 +54,9 @@ properties:
               - renesas,vin-r8a77995 # R-Car D3
               - renesas,vin-r8a779a0 # R-Car V3U
               - renesas,vin-r8a779g0 # R-Car V4H
+      - items:
+          - const: renesas,vin-r8a779h0 # R-Car V4L2
+          - const: renesas,vin-r8a779g0 # R-Car V4H
 
   reg:
     maxItems: 1

Which requires no driver or dts changes. That could become:
      - items:
          - enum:
              - renesas,vin-r8a779h0 # R-Car V4L2
              - renesas,vin-r8a779i0 # R-Car R4P17
          - const: renesas,vin-r8a779g0 # R-Car V4H

if there's another compatible device in the future.

> Apologies again for the confusion.

dw about it

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Niklas Söderlund]

On 2024-06-20 17:27:00 +0100, Conor Dooley wrote:

> > +      - items:
> > +          - enum:
> >                - renesas,vin-r8a779g0 # R-Car V4H
> > +              - renesas,vin-r8a779h0 # R-Car V4M
> > +          - const: renesas,rcar-gen4-vin # Generic R-Car Gen4
> > 
> > If so I can see that working as I could still fix any issues that come 
> > from differences between V4H and V4M if needed. If so do you think it 
> > best to add this in two different patches? One to add the 
> > renesas,rcar-gen4-vin fallback (which will also need DTS updates to fix 
> > warnings from exciting users of V4H not listing the gen4 fallback) and 
> > one to add V4M?
> 
> 
> I would just do:
> diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> index 5539d0f8e74d..22bbad42fc03 100644
> --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> @@ -54,6 +54,9 @@ properties:
>                - renesas,vin-r8a77995 # R-Car D3
>                - renesas,vin-r8a779a0 # R-Car V3U
>                - renesas,vin-r8a779g0 # R-Car V4H
> +      - items:
> +          - const: renesas,vin-r8a779h0 # R-Car V4L2
> +          - const: renesas,vin-r8a779g0 # R-Car V4H

@Geert: What do you think about this? This would be a first use-case for 
compatibles crossing SoC DTS files that I know of. I'm a bit uneasy 
going down this road.

Would this not also effect the existing users of renesas,vin-r8a779g0 
which would now need something similar to what you propose below with a 
list of SoC compatibles and a fallback.

>  
>    reg:
>      maxItems: 1
> 
> Which requires no driver or dts changes. That could become:
>       - items:
>           - enum:
>               - renesas,vin-r8a779h0 # R-Car V4L2
>               - renesas,vin-r8a779i0 # R-Car R4P17
>           - const: renesas,vin-r8a779g0 # R-Car V4H

FWIW, on Gen2 where fallback es where useful compared to Gen3 we did 
this with "renesas,rcar-gen2-vin".

> 
> if there's another compatible device in the future.
> 
> > Apologies again for the confusion.
> 
> dw about it



-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v3 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Geert Uytterhoeven]

Hi Niklas,

On Thu, Jun 20, 2024 at 7:22 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> On 2024-06-20 17:27:00 +0100, Conor Dooley wrote:
> > > +      - items:
> > > +          - enum:
> > >                - renesas,vin-r8a779g0 # R-Car V4H
> > > +              - renesas,vin-r8a779h0 # R-Car V4M
> > > +          - const: renesas,rcar-gen4-vin # Generic R-Car Gen4
> > >
> > > If so I can see that working as I could still fix any issues that come
> > > from differences between V4H and V4M if needed. If so do you think it
> > > best to add this in two different patches? One to add the
> > > renesas,rcar-gen4-vin fallback (which will also need DTS updates to fix
> > > warnings from exciting users of V4H not listing the gen4 fallback) and
> > > one to add V4M?
> >
> >
> > I would just do:
> > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > index 5539d0f8e74d..22bbad42fc03 100644
> > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > @@ -54,6 +54,9 @@ properties:
> >                - renesas,vin-r8a77995 # R-Car D3
> >                - renesas,vin-r8a779a0 # R-Car V3U
> >                - renesas,vin-r8a779g0 # R-Car V4H
> > +      - items:
> > +          - const: renesas,vin-r8a779h0 # R-Car V4L2
> > +          - const: renesas,vin-r8a779g0 # R-Car V4H
>
> @Geert: What do you think about this? This would be a first use-case for
> compatibles crossing SoC DTS files that I know of. I'm a bit uneasy
> going down this road.

Me too ;-)

> Would this not also effect the existing users of renesas,vin-r8a779g0
> which would now need something similar to what you propose below with a
> list of SoC compatibles and a fallback.
>
> >
> >    reg:
> >      maxItems: 1
> >
> > Which requires no driver or dts changes. That could become:
> >       - items:
> >           - enum:
> >               - renesas,vin-r8a779h0 # R-Car V4L2
> >               - renesas,vin-r8a779i0 # R-Car R4P17
> >           - const: renesas,vin-r8a779g0 # R-Car V4H
>
> FWIW, on Gen2 where fallback es where useful compared to Gen3 we did
> this with "renesas,rcar-gen2-vin".

We do know there are differences (albeit probably small) among the R-Car
Gen4 VIN implementations, so I am reluctant to add a family-specific
compatible value.  Typically we only use a family-specific compatible
value if the IP cores are known (or better, assumed ;-) to be identical.

And sometimes our assumptions turn out to be wrong...
See slides 25-33 (last two for the numbers) of my talk at ER2019
https://embedded-recipes.org/2019/talks/herd-your-socs-become-a-matchmaker/

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH v3 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Niklas Söderlund]

Hi Conor,

On 2024-06-21 09:21:24 +0200, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Thu, Jun 20, 2024 at 7:22 PM Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > On 2024-06-20 17:27:00 +0100, Conor Dooley wrote:
> > > > +      - items:
> > > > +          - enum:
> > > >                - renesas,vin-r8a779g0 # R-Car V4H
> > > > +              - renesas,vin-r8a779h0 # R-Car V4M
> > > > +          - const: renesas,rcar-gen4-vin # Generic R-Car Gen4
> > > >
> > > > If so I can see that working as I could still fix any issues that come
> > > > from differences between V4H and V4M if needed. If so do you think it
> > > > best to add this in two different patches? One to add the
> > > > renesas,rcar-gen4-vin fallback (which will also need DTS updates to fix
> > > > warnings from exciting users of V4H not listing the gen4 fallback) and
> > > > one to add V4M?
> > >
> > >
> > > I would just do:
> > > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > index 5539d0f8e74d..22bbad42fc03 100644
> > > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > @@ -54,6 +54,9 @@ properties:
> > >                - renesas,vin-r8a77995 # R-Car D3
> > >                - renesas,vin-r8a779a0 # R-Car V3U
> > >                - renesas,vin-r8a779g0 # R-Car V4H
> > > +      - items:
> > > +          - const: renesas,vin-r8a779h0 # R-Car V4L2
> > > +          - const: renesas,vin-r8a779g0 # R-Car V4H
> >
> > @Geert: What do you think about this? This would be a first use-case for
> > compatibles crossing SoC DTS files that I know of. I'm a bit uneasy
> > going down this road.
> 
> Me too ;-)
> 
> > Would this not also effect the existing users of renesas,vin-r8a779g0
> > which would now need something similar to what you propose below with a
> > list of SoC compatibles and a fallback.
> >
> > >
> > >    reg:
> > >      maxItems: 1
> > >
> > > Which requires no driver or dts changes. That could become:
> > >       - items:
> > >           - enum:
> > >               - renesas,vin-r8a779h0 # R-Car V4L2
> > >               - renesas,vin-r8a779i0 # R-Car R4P17
> > >           - const: renesas,vin-r8a779g0 # R-Car V4H
> >
> > FWIW, on Gen2 where fallback es where useful compared to Gen3 we did
> > this with "renesas,rcar-gen2-vin".
> 
> We do know there are differences (albeit probably small) among the R-Car
> Gen4 VIN implementations, so I am reluctant to add a family-specific
> compatible value.  Typically we only use a family-specific compatible
> value if the IP cores are known (or better, assumed ;-) to be identical.
> 
> And sometimes our assumptions turn out to be wrong...
> See slides 25-33 (last two for the numbers) of my talk at ER2019
> https://embedded-recipes.org/2019/talks/herd-your-socs-become-a-matchmaker/

Do Geert's slides help to explain the R-Car perspective on why a 
family-specific fallback compatible might not be desirable, and why the 
SoC specific one is proposed? 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v3 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3625 bytes --]

On Mon, Jun 24, 2024 at 11:20:29AM +0200, Niklas Söderlund wrote:
> Hi Conor,
> 
> On 2024-06-21 09:21:24 +0200, Geert Uytterhoeven wrote:
> > Hi Niklas,
> > 
> > On Thu, Jun 20, 2024 at 7:22 PM Niklas Söderlund
> > <niklas.soderlund+renesas@ragnatech.se> wrote:
> > > On 2024-06-20 17:27:00 +0100, Conor Dooley wrote:
> > > > > +      - items:
> > > > > +          - enum:
> > > > >                - renesas,vin-r8a779g0 # R-Car V4H
> > > > > +              - renesas,vin-r8a779h0 # R-Car V4M
> > > > > +          - const: renesas,rcar-gen4-vin # Generic R-Car Gen4
> > > > >
> > > > > If so I can see that working as I could still fix any issues that come
> > > > > from differences between V4H and V4M if needed. If so do you think it
> > > > > best to add this in two different patches? One to add the
> > > > > renesas,rcar-gen4-vin fallback (which will also need DTS updates to fix
> > > > > warnings from exciting users of V4H not listing the gen4 fallback) and
> > > > > one to add V4M?
> > > >
> > > >
> > > > I would just do:
> > > > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > index 5539d0f8e74d..22bbad42fc03 100644
> > > > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > @@ -54,6 +54,9 @@ properties:
> > > >                - renesas,vin-r8a77995 # R-Car D3
> > > >                - renesas,vin-r8a779a0 # R-Car V3U
> > > >                - renesas,vin-r8a779g0 # R-Car V4H
> > > > +      - items:
> > > > +          - const: renesas,vin-r8a779h0 # R-Car V4L2
> > > > +          - const: renesas,vin-r8a779g0 # R-Car V4H
> > >
> > > @Geert: What do you think about this? This would be a first use-case for
> > > compatibles crossing SoC DTS files that I know of. I'm a bit uneasy
> > > going down this road.
> > 
> > Me too ;-)
> > 
> > > Would this not also effect the existing users of renesas,vin-r8a779g0
> > > which would now need something similar to what you propose below with a
> > > list of SoC compatibles and a fallback.
> > >
> > > >
> > > >    reg:
> > > >      maxItems: 1
> > > >
> > > > Which requires no driver or dts changes. That could become:
> > > >       - items:
> > > >           - enum:
> > > >               - renesas,vin-r8a779h0 # R-Car V4L2
> > > >               - renesas,vin-r8a779i0 # R-Car R4P17
> > > >           - const: renesas,vin-r8a779g0 # R-Car V4H
> > >
> > > FWIW, on Gen2 where fallback es where useful compared to Gen3 we did
> > > this with "renesas,rcar-gen2-vin".
> > 
> > We do know there are differences (albeit probably small) among the R-Car
> > Gen4 VIN implementations, so I am reluctant to add a family-specific
> > compatible value.  Typically we only use a family-specific compatible
> > value if the IP cores are known (or better, assumed ;-) to be identical.
> > 
> > And sometimes our assumptions turn out to be wrong...
> > See slides 25-33 (last two for the numbers) of my talk at ER2019
> > https://embedded-recipes.org/2019/talks/herd-your-socs-become-a-matchmaker/
> 
> Do Geert's slides help to explain the R-Car perspective on why a 
> family-specific fallback compatible might not be desirable, and why the 
> SoC specific one is proposed? 

IIRC, it was you that wanted to use a "family-specific" fallback, I
don't understand what you want from me. If you look back at even the
context in this email, you can see you suggesting one and my counter
point.

Confused,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Niklas Söderlund]

On 2024-06-24 11:36:40 +0100, Conor Dooley wrote:
> On Mon, Jun 24, 2024 at 11:20:29AM +0200, Niklas Söderlund wrote:
> > Hi Conor,
> > 
> > On 2024-06-21 09:21:24 +0200, Geert Uytterhoeven wrote:
> > > Hi Niklas,
> > > 
> > > On Thu, Jun 20, 2024 at 7:22 PM Niklas Söderlund
> > > <niklas.soderlund+renesas@ragnatech.se> wrote:
> > > > On 2024-06-20 17:27:00 +0100, Conor Dooley wrote:
> > > > > > +      - items:
> > > > > > +          - enum:
> > > > > >                - renesas,vin-r8a779g0 # R-Car V4H
> > > > > > +              - renesas,vin-r8a779h0 # R-Car V4M
> > > > > > +          - const: renesas,rcar-gen4-vin # Generic R-Car Gen4
> > > > > >
> > > > > > If so I can see that working as I could still fix any issues that come
> > > > > > from differences between V4H and V4M if needed. If so do you think it
> > > > > > best to add this in two different patches? One to add the
> > > > > > renesas,rcar-gen4-vin fallback (which will also need DTS updates to fix
> > > > > > warnings from exciting users of V4H not listing the gen4 fallback) and
> > > > > > one to add V4M?
> > > > >
> > > > >
> > > > > I would just do:
> > > > > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > index 5539d0f8e74d..22bbad42fc03 100644
> > > > > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > @@ -54,6 +54,9 @@ properties:
> > > > >                - renesas,vin-r8a77995 # R-Car D3
> > > > >                - renesas,vin-r8a779a0 # R-Car V3U
> > > > >                - renesas,vin-r8a779g0 # R-Car V4H
> > > > > +      - items:
> > > > > +          - const: renesas,vin-r8a779h0 # R-Car V4L2
> > > > > +          - const: renesas,vin-r8a779g0 # R-Car V4H
> > > >
> > > > @Geert: What do you think about this? This would be a first use-case for
> > > > compatibles crossing SoC DTS files that I know of. I'm a bit uneasy
> > > > going down this road.
> > > 
> > > Me too ;-)
> > > 
> > > > Would this not also effect the existing users of renesas,vin-r8a779g0
> > > > which would now need something similar to what you propose below with a
> > > > list of SoC compatibles and a fallback.
> > > >
> > > > >
> > > > >    reg:
> > > > >      maxItems: 1
> > > > >
> > > > > Which requires no driver or dts changes. That could become:
> > > > >       - items:
> > > > >           - enum:
> > > > >               - renesas,vin-r8a779h0 # R-Car V4L2
> > > > >               - renesas,vin-r8a779i0 # R-Car R4P17
> > > > >           - const: renesas,vin-r8a779g0 # R-Car V4H
> > > >
> > > > FWIW, on Gen2 where fallback es where useful compared to Gen3 we did
> > > > this with "renesas,rcar-gen2-vin".
> > > 
> > > We do know there are differences (albeit probably small) among the R-Car
> > > Gen4 VIN implementations, so I am reluctant to add a family-specific
> > > compatible value.  Typically we only use a family-specific compatible
> > > value if the IP cores are known (or better, assumed ;-) to be identical.
> > > 
> > > And sometimes our assumptions turn out to be wrong...
> > > See slides 25-33 (last two for the numbers) of my talk at ER2019
> > > https://embedded-recipes.org/2019/talks/herd-your-socs-become-a-matchmaker/
> > 
> > Do Geert's slides help to explain the R-Car perspective on why a 
> > family-specific fallback compatible might not be desirable, and why the 
> > SoC specific one is proposed? 
> 
> IIRC, it was you that wanted to use a "family-specific" fallback, I
> don't understand what you want from me. If you look back at even the
> context in this email, you can see you suggesting one and my counter
> point.

Sorry that I'm spreading my confusion around and taking up your time.  
I'm trying to understand if Geert's reply helped outline why a single 
SoC specific compatible is being used here, if so I was hoping a revised 
commit message would make this solution acceptable.  

If not I will try to summaries the issue and the different proposals so 
we can find a design that works and address some of the confusion before 
sending a new version.

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v3 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5175 bytes --]

On Mon, Jun 24, 2024 at 02:50:51PM +0200, Niklas Söderlund wrote:
> On 2024-06-24 11:36:40 +0100, Conor Dooley wrote:
> > On Mon, Jun 24, 2024 at 11:20:29AM +0200, Niklas Söderlund wrote:
> > > Hi Conor,
> > > 
> > > On 2024-06-21 09:21:24 +0200, Geert Uytterhoeven wrote:
> > > > Hi Niklas,
> > > > 
> > > > On Thu, Jun 20, 2024 at 7:22 PM Niklas Söderlund
> > > > <niklas.soderlund+renesas@ragnatech.se> wrote:
> > > > > On 2024-06-20 17:27:00 +0100, Conor Dooley wrote:
> > > > > > > +      - items:
> > > > > > > +          - enum:
> > > > > > >                - renesas,vin-r8a779g0 # R-Car V4H
> > > > > > > +              - renesas,vin-r8a779h0 # R-Car V4M
> > > > > > > +          - const: renesas,rcar-gen4-vin # Generic R-Car Gen4
> > > > > > >
> > > > > > > If so I can see that working as I could still fix any issues that come
> > > > > > > from differences between V4H and V4M if needed. If so do you think it
> > > > > > > best to add this in two different patches? One to add the
> > > > > > > renesas,rcar-gen4-vin fallback (which will also need DTS updates to fix
> > > > > > > warnings from exciting users of V4H not listing the gen4 fallback) and
> > > > > > > one to add V4M?
> > > > > >
> > > > > >
> > > > > > I would just do:
> > > > > > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > > index 5539d0f8e74d..22bbad42fc03 100644
> > > > > > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > > @@ -54,6 +54,9 @@ properties:
> > > > > >                - renesas,vin-r8a77995 # R-Car D3
> > > > > >                - renesas,vin-r8a779a0 # R-Car V3U
> > > > > >                - renesas,vin-r8a779g0 # R-Car V4H
> > > > > > +      - items:
> > > > > > +          - const: renesas,vin-r8a779h0 # R-Car V4L2
> > > > > > +          - const: renesas,vin-r8a779g0 # R-Car V4H
> > > > >
> > > > > @Geert: What do you think about this? This would be a first use-case for
> > > > > compatibles crossing SoC DTS files that I know of. I'm a bit uneasy
> > > > > going down this road.
> > > > 
> > > > Me too ;-)
> > > > 
> > > > > Would this not also effect the existing users of renesas,vin-r8a779g0
> > > > > which would now need something similar to what you propose below with a
> > > > > list of SoC compatibles and a fallback.
> > > > >
> > > > > >
> > > > > >    reg:
> > > > > >      maxItems: 1
> > > > > >
> > > > > > Which requires no driver or dts changes. That could become:
> > > > > >       - items:
> > > > > >           - enum:
> > > > > >               - renesas,vin-r8a779h0 # R-Car V4L2
> > > > > >               - renesas,vin-r8a779i0 # R-Car R4P17
> > > > > >           - const: renesas,vin-r8a779g0 # R-Car V4H
> > > > >
> > > > > FWIW, on Gen2 where fallback es where useful compared to Gen3 we did
> > > > > this with "renesas,rcar-gen2-vin".
> > > > 
> > > > We do know there are differences (albeit probably small) among the R-Car
> > > > Gen4 VIN implementations, so I am reluctant to add a family-specific
> > > > compatible value.  Typically we only use a family-specific compatible
> > > > value if the IP cores are known (or better, assumed ;-) to be identical.
> > > > 
> > > > And sometimes our assumptions turn out to be wrong...
> > > > See slides 25-33 (last two for the numbers) of my talk at ER2019
> > > > https://embedded-recipes.org/2019/talks/herd-your-socs-become-a-matchmaker/
> > > 
> > > Do Geert's slides help to explain the R-Car perspective on why a 
> > > family-specific fallback compatible might not be desirable, and why the 
> > > SoC specific one is proposed? 
> > 
> > IIRC, it was you that wanted to use a "family-specific" fallback, I
> > don't understand what you want from me. If you look back at even the
> > context in this email, you can see you suggesting one and my counter
> > point.
> 
> Sorry that I'm spreading my confusion around and taking up your time.

I don't care if non-native speakers of English say confusing things,
don't worry about that.

> I'm trying to understand if Geert's reply helped outline why a single 
> SoC specific compatible is being used here, if so I was hoping a revised 
> commit message would make this solution acceptable.  
> 
> If not I will try to summaries the issue and the different proposals so 
> we can find a design that works and address some of the confusion before 
> sending a new version.

These devices look, for all intents and purposes, to be compatible. If
they're not, *say* what is not compatible about them. Don't just say
"ohh there might be, but they're small", say exactly what - because your
driver makes them look compatible. It looks compatible with the a0 as
well... The vibe that comes across here is of being "afraid" of having
fallback compatibles that reference other SoCs - which is totally normal
for other vendors, not that there are any differences in programming
model between the VIN instances on these devices.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v3 2/2] media: rcar-vin: Add support for R-Car V4M

[Niklas Söderlund]

Add support for R-Car V4M. The V4M is similar to the other Gen4 SoC
supported V4H, and uses the ISP Channel Selector as its only possible
video input source.

The datasheet for the two SoCs have small nuances around the Pre-Clip
registers ELPrC and EPPrC in three use-cases, interlaced images,
embedded data and RAW8 input. On V4H the values written to the registers
are based on odd numbers while on V4M they are even numbers, based on
the input image size.

No board that uses these SoCs which also have the external peripherals
to test these nuances exists. Most likely this is an issue in the
datasheet, but to make this easy to address in the future do not add a
common Gen4 fallback compatible. Instead uses SoC specific compatibles
for both SoCs. This is what was done for Gen3 SoCs, which also had
similar nuances in the register documentation.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
* Changes since v2
- Extend commit message to explain why SoC specific compatibles are use
  in the driver instead of adding a new Gen4 fallback to cover both V4H
  and V4M.
- Add review tags.

* Changes since v1
- Create a shared Gen4 info strucutre.
---
 drivers/media/platform/renesas/rcar-vin/rcar-core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
index 809c3a38cc4a..6992b61f0d48 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
@@ -1283,7 +1283,7 @@ static const struct rvin_info rcar_info_r8a779a0 = {
 	.max_height = 4096,
 };
 
-static const struct rvin_info rcar_info_r8a779g0 = {
+static const struct rvin_info rcar_info_gen4 = {
 	.model = RCAR_GEN3,
 	.use_mc = true,
 	.use_isp = true,
@@ -1359,7 +1359,11 @@ static const struct of_device_id rvin_of_id_table[] = {
 	},
 	{
 		.compatible = "renesas,vin-r8a779g0",
-		.data = &rcar_info_r8a779g0,
+		.data = &rcar_info_gen4,
+	},
+	{
+		.compatible = "renesas,vin-r8a779h0",
+		.data = &rcar_info_gen4,
 	},
 	{ /* Sentinel */ },
 };
-- 
2.45.2