[v2,01/19] dt-bindings: display: renesas, cmm: Add R-Car CMM documentation

Commit Message

Jacopo Mondi July 6, 2019, 2:07 p.m. UTC

Add device tree bindings documentation for the Renesas R-Car Display
Unit Color Management Module.

CMM is the image enhancement module available on each R-Car DU video
channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).

Signed-off-by: Jacopo Mondi <[e-mail]>
Reviewed-by: Laurent Pinchart <[e-mail]>
---
 .../bindings/display/renesas,cmm.txt          | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/renesas,cmm.txt

Comments

Geert Uytterhoeven July 8, 2019, 7:58 a.m. UTC | #1

Hi Jacopo,

On Sat, Jul 6, 2019 at 4:07 PM Jacopo Mondi <[e-mail]> wrote:
> Add device tree bindings documentation for the Renesas R-Car Display
> Unit Color Management Module.
>
> CMM is the image enhancement module available on each R-Car DU video
> channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
>
> Signed-off-by: Jacopo Mondi <[e-mail]>
> Reviewed-by: Laurent Pinchart <[e-mail]>

Thanks for your patch!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> @@ -0,0 +1,25 @@
> +* Renesas R-Car Color Management Module (CMM)
> +
> +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> +
> +Required properties:
> + - compatible: shall be one of:
> +   - "renesas,rcar-gen3-cmm"
> +   - "renesas,rcar-gen2-cmm"

Why do you think you do not need SoC-specific compatible values?
What if you discover a different across the R-Car Gen3 line tomorrow?
Does the IP block have a version register?

Gr{oetje,eeting}s,

                        Geert

Geert Uytterhoeven Aug. 19, 2019, 1:45 p.m. UTC | #2

Hi Jacopo,

On Mon, Jul 8, 2019 at 9:58 AM Geert Uytterhoeven <[e-mail]> wrote:
> On Sat, Jul 6, 2019 at 4:07 PM Jacopo Mondi <[e-mail]> wrote:
> > Add device tree bindings documentation for the Renesas R-Car Display
> > Unit Color Management Module.
> >
> > CMM is the image enhancement module available on each R-Car DU video
> > channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
> >
> > Signed-off-by: Jacopo Mondi <[e-mail]>
> > Reviewed-by: Laurent Pinchart <[e-mail]>
>
> Thanks for your patch!
>
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> > @@ -0,0 +1,25 @@
> > +* Renesas R-Car Color Management Module (CMM)
> > +
> > +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> > +
> > +Required properties:
> > + - compatible: shall be one of:
> > +   - "renesas,rcar-gen3-cmm"
> > +   - "renesas,rcar-gen2-cmm"
>
> Why do you think you do not need SoC-specific compatible values?
> What if you discover a different across the R-Car Gen3 line tomorrow?
> Does the IP block have a version register?

Do you have an answer to these questions?
Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

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

Jacopo Mondi Aug. 20, 2019, 7:48 a.m. UTC | #3

Hi Geert,
   sorry for the delayed response..

On Mon, Aug 19, 2019 at 03:45:54PM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Mon, Jul 8, 2019 at 9:58 AM Geert Uytterhoeven <[e-mail]> wrote:
> > On Sat, Jul 6, 2019 at 4:07 PM Jacopo Mondi <[e-mail]> wrote:
> > > Add device tree bindings documentation for the Renesas R-Car Display
> > > Unit Color Management Module.
> > >
> > > CMM is the image enhancement module available on each R-Car DU video
> > > channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
> > >
> > > Signed-off-by: Jacopo Mondi <[e-mail]>
> > > Reviewed-by: Laurent Pinchart <[e-mail]>
> >
> > Thanks for your patch!
> >
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> > > @@ -0,0 +1,25 @@
> > > +* Renesas R-Car Color Management Module (CMM)
> > > +
> > > +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> > > +
> > > +Required properties:
> > > + - compatible: shall be one of:
> > > +   - "renesas,rcar-gen3-cmm"
> > > +   - "renesas,rcar-gen2-cmm"
> >
> > Why do you think you do not need SoC-specific compatible values?
> > What if you discover a different across the R-Car Gen3 line tomorrow?
> > Does the IP block have a version register?
>
> Do you have an answer to these questions?

It does not seem to me that CMM has any version register, nor there
are differences between the different Gen3 SoCs..

However, even if we now define a single compatible property for
gen3/gen2 and we later find out one of the SoC needs a soc-specific
property we can safely add it and keep the generic gen3/gen2 one as
fallback.. Does it work for you?

Thanks
   j


> Thanks!
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]
>
> 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

Geert Uytterhoeven Aug. 20, 2019, 7:53 a.m. UTC | #4

Hi Jacopo,

On Tue, Aug 20, 2019 at 9:47 AM Jacopo Mondi <[e-mail]> wrote:
> On Mon, Aug 19, 2019 at 03:45:54PM +0200, Geert Uytterhoeven wrote:
> > On Mon, Jul 8, 2019 at 9:58 AM Geert Uytterhoeven <[e-mail]> wrote:
> > > On Sat, Jul 6, 2019 at 4:07 PM Jacopo Mondi <[e-mail]> wrote:
> > > > Add device tree bindings documentation for the Renesas R-Car Display
> > > > Unit Color Management Module.
> > > >
> > > > CMM is the image enhancement module available on each R-Car DU video
> > > > channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
> > > >
> > > > Signed-off-by: Jacopo Mondi <[e-mail]>
> > > > Reviewed-by: Laurent Pinchart <[e-mail]>
> > >
> > > Thanks for your patch!
> > >
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> > > > @@ -0,0 +1,25 @@
> > > > +* Renesas R-Car Color Management Module (CMM)
> > > > +
> > > > +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> > > > +
> > > > +Required properties:
> > > > + - compatible: shall be one of:
> > > > +   - "renesas,rcar-gen3-cmm"
> > > > +   - "renesas,rcar-gen2-cmm"
> > >
> > > Why do you think you do not need SoC-specific compatible values?
> > > What if you discover a different across the R-Car Gen3 line tomorrow?
> > > Does the IP block have a version register?
> >
> > Do you have an answer to these questions?
>
> It does not seem to me that CMM has any version register, nor there
> are differences between the different Gen3 SoCs..
>
> However, even if we now define a single compatible property for
> gen3/gen2 and we later find out one of the SoC needs a soc-specific
> property we can safely add it and keep the generic gen3/gen2 one as
> fallback.. Does it work for you?

Unfortunately that won't work, as the existing DTBs won't have the
soc-specific compatible value.
You could still resort to soc_device_match(), but it is better to avoid that.

Gr{oetje,eeting}s,

                        Geert

Jacopo Mondi Aug. 20, 2019, 8:05 a.m. UTC | #5

Hi Geert,

On Tue, Aug 20, 2019 at 09:53:44AM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Tue, Aug 20, 2019 at 9:47 AM Jacopo Mondi <[e-mail]> wrote:
> > On Mon, Aug 19, 2019 at 03:45:54PM +0200, Geert Uytterhoeven wrote:
> > > On Mon, Jul 8, 2019 at 9:58 AM Geert Uytterhoeven <[e-mail]> wrote:
> > > > On Sat, Jul 6, 2019 at 4:07 PM Jacopo Mondi <[e-mail]> wrote:
> > > > > Add device tree bindings documentation for the Renesas R-Car Display
> > > > > Unit Color Management Module.
> > > > >
> > > > > CMM is the image enhancement module available on each R-Car DU video
> > > > > channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <[e-mail]>
> > > > > Reviewed-by: Laurent Pinchart <[e-mail]>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> > > > > @@ -0,0 +1,25 @@
> > > > > +* Renesas R-Car Color Management Module (CMM)
> > > > > +
> > > > > +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> > > > > +
> > > > > +Required properties:
> > > > > + - compatible: shall be one of:
> > > > > +   - "renesas,rcar-gen3-cmm"
> > > > > +   - "renesas,rcar-gen2-cmm"
> > > >
> > > > Why do you think you do not need SoC-specific compatible values?
> > > > What if you discover a different across the R-Car Gen3 line tomorrow?
> > > > Does the IP block have a version register?
> > >
> > > Do you have an answer to these questions?
> >
> > It does not seem to me that CMM has any version register, nor there
> > are differences between the different Gen3 SoCs..
> >
> > However, even if we now define a single compatible property for
> > gen3/gen2 and we later find out one of the SoC needs a soc-specific
> > property we can safely add it and keep the generic gen3/gen2 one as
> > fallback.. Does it work for you?
>
> Unfortunately that won't work, as the existing DTBs won't have the
> soc-specific compatible value.

Correct, existing dtbs won't have the soc-specific value... However,
there are functional differences between different SoCs according to
the datasheet, but if it's good practice to provide soc-specific
compatibles "just in case" I'm fine doing that..


> You could still resort to soc_device_match(), but it is better to avoid that.

I see... Also that function's documentation prescribes to go through
DT first, so I guess it's our last resort...


>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]
>
> 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

Laurent Pinchart Aug. 20, 2019, 5:41 p.m. UTC | #6

Hi Geert,

On Tue, Aug 20, 2019 at 09:53:44AM +0200, Geert Uytterhoeven wrote:
> On Tue, Aug 20, 2019 at 9:47 AM Jacopo Mondi <[e-mail]> wrote:
> > On Mon, Aug 19, 2019 at 03:45:54PM +0200, Geert Uytterhoeven wrote:
> >> On Mon, Jul 8, 2019 at 9:58 AM Geert Uytterhoeven <[e-mail]> wrote:
> >>> On Sat, Jul 6, 2019 at 4:07 PM Jacopo Mondi <[e-mail]> wrote:
> >>>> Add device tree bindings documentation for the Renesas R-Car Display
> >>>> Unit Color Management Module.
> >>>>
> >>>> CMM is the image enhancement module available on each R-Car DU video
> >>>> channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
> >>>>
> >>>> Signed-off-by: Jacopo Mondi <[e-mail]>
> >>>> Reviewed-by: Laurent Pinchart <[e-mail]>
> >>>
> >>> Thanks for your patch!
> >>>
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> >>>> @@ -0,0 +1,25 @@
> >>>> +* Renesas R-Car Color Management Module (CMM)
> >>>> +
> >>>> +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> >>>> +
> >>>> +Required properties:
> >>>> + - compatible: shall be one of:
> >>>> +   - "renesas,rcar-gen3-cmm"
> >>>> +   - "renesas,rcar-gen2-cmm"
> >>>
> >>> Why do you think you do not need SoC-specific compatible values?
> >>> What if you discover a different across the R-Car Gen3 line tomorrow?
> >>> Does the IP block have a version register?
> >>
> >> Do you have an answer to these questions?
> >
> > It does not seem to me that CMM has any version register, nor there
> > are differences between the different Gen3 SoCs..
> >
> > However, even if we now define a single compatible property for
> > gen3/gen2 and we later find out one of the SoC needs a soc-specific
> > property we can safely add it and keep the generic gen3/gen2 one as
> > fallback.. Does it work for you?
> 
> Unfortunately that won't work, as the existing DTBs won't have the
> soc-specific compatible value.
> You could still resort to soc_device_match(), but it is better to avoid that.

We've had the same discussion over and over for quite a long time :-) I
wonder, now that we have implemented SoC-specific compatible values for
many IP cores, how many of them have actually benefited from it ? I'm
not considering IP cores where we knew from the start that each SoC was
different (such as pinctrl or clocks for instance), but IP cores where
we though all SoCs would be handled in the same way. I also wouldn't
count ES-specific differences, as those are handled by
soc_device_match() anyway.

Geert Uytterhoeven Aug. 20, 2019, 6:08 p.m. UTC | #7

Hi Laurent,

On Tue, Aug 20, 2019 at 7:41 PM Laurent Pinchart
<[e-mail]> wrote:
> On Tue, Aug 20, 2019 at 09:53:44AM +0200, Geert Uytterhoeven wrote:
> > On Tue, Aug 20, 2019 at 9:47 AM Jacopo Mondi <[e-mail]> wrote:
> > > On Mon, Aug 19, 2019 at 03:45:54PM +0200, Geert Uytterhoeven wrote:
> > >> On Mon, Jul 8, 2019 at 9:58 AM Geert Uytterhoeven <[e-mail]> wrote:
> > >>> On Sat, Jul 6, 2019 at 4:07 PM Jacopo Mondi <[e-mail]> wrote:
> > >>>> Add device tree bindings documentation for the Renesas R-Car Display
> > >>>> Unit Color Management Module.
> > >>>>
> > >>>> CMM is the image enhancement module available on each R-Car DU video
> > >>>> channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
> > >>>>
> > >>>> Signed-off-by: Jacopo Mondi <[e-mail]>
> > >>>> Reviewed-by: Laurent Pinchart <[e-mail]>
> > >>>
> > >>> Thanks for your patch!
> > >>>
> > >>>> --- /dev/null
> > >>>> +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> > >>>> @@ -0,0 +1,25 @@
> > >>>> +* Renesas R-Car Color Management Module (CMM)
> > >>>> +
> > >>>> +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> > >>>> +
> > >>>> +Required properties:
> > >>>> + - compatible: shall be one of:
> > >>>> +   - "renesas,rcar-gen3-cmm"
> > >>>> +   - "renesas,rcar-gen2-cmm"
> > >>>
> > >>> Why do you think you do not need SoC-specific compatible values?
> > >>> What if you discover a different across the R-Car Gen3 line tomorrow?
> > >>> Does the IP block have a version register?
> > >>
> > >> Do you have an answer to these questions?
> > >
> > > It does not seem to me that CMM has any version register, nor there
> > > are differences between the different Gen3 SoCs..
> > >
> > > However, even if we now define a single compatible property for
> > > gen3/gen2 and we later find out one of the SoC needs a soc-specific
> > > property we can safely add it and keep the generic gen3/gen2 one as
> > > fallback.. Does it work for you?
> >
> > Unfortunately that won't work, as the existing DTBs won't have the
> > soc-specific compatible value.
> > You could still resort to soc_device_match(), but it is better to avoid that.
>
> We've had the same discussion over and over for quite a long time :-) I
> wonder, now that we have implemented SoC-specific compatible values for
> many IP cores, how many of them have actually benefited from it ? I'm
> not considering IP cores where we knew from the start that each SoC was
> different (such as pinctrl or clocks for instance), but IP cores where
> we though all SoCs would be handled in the same way. I also wouldn't
> count ES-specific differences, as those are handled by
> soc_device_match() anyway.

Thank you for making me dive into this ;-)

For R-Car Gen3 only:

DRIF?
The driver still matches against "renesas,rcar-gen3-drif", but we found a
difference on M3-N (didn't check if that was documented initially or not).

PCIe?
IIRC, there is still a special PHY register on one of the V3x SoCs that
the driver doesn't handle yet, and wasn't documented initially.

RPC-IF?
Lots of small differences (you can claim they were documented, but they
were unexpected), and non-documented different magic values in the
(not yet upstreamed) driver.

SOUND?
R-Car E3 special handling was bolted on later.

Thermal?
M3-W turned out to have a different Tj than the other SoCs using the same
thermal module (yeah, thermal is a bad example, as some Gen3 SoCs use
the Gen2 thermal module, so we needed to differentiate anyway).

USBHS?
Initially we thought Gen3 was identical to Gen2.  Later it turned out that
(1) wasn't true, and (2) E3/D3 used a different PLL than the other Gen3
SoCs.

VIN?
IIRC, we initialize thought a family-specific compatible value would work
for Gen3, like it did for Gen2, but had to reconsider.

I may have missed some...

Convinced?

Gr{oetje,eeting}s,

                        Geert

Patch

diff --git a/Documentation/devicetree/bindings/display/renesas,cmm.txt b/Documentation/devicetree/bindings/display/renesas,cmm.txt
new file mode 100644
index 000000000000..083dc1357b2b
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
@@ -0,0 +1,25 @@ 
+* Renesas R-Car Color Management Module (CMM)
+
+Renesas R-Car image enhancement module connected to R-Car DU video channels.
+
+Required properties:
+ - compatible: shall be one of:
+   - "renesas,rcar-gen3-cmm"
+   - "renesas,rcar-gen2-cmm"
+
+ - reg: the address base and length of the memory area where CMM control
+   registers are mapped to.
+
+ - clocks: phandle and clock-specifier pair to the CMM functional clock
+   supplier.
+
+Example:
+--------
+
+	cmm0: cmm@fea40000 {
+		compatible = "renesas,rcar-gen3-cmm";
+		reg = <0 0xfea40000 0 0x1000>;
+		power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
+		clocks = <&cpg CPG_MOD 711>;
+		resets = <&cpg 711>;
+	};