[RFC,2/9] dt-bindings: display, renesas,du: Document cmms property
diff mbox series

Message ID 20190508173428.22054-3-jacopo+renesas@jmondi.org
State New
Delegated to: Kieran Bingham
Headers show
Series
  • drm: rcar-du: Add CMM support to M3-W (plumbing only)
Related show

Commit Message

Jacopo Mondi May 8, 2019, 5:34 p.m. UTC
Document the newly added 'cmms' property which accepts a list of phandle
and channel index pairs that point to the CMM units available for each
Display Unit output video channel.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 Documentation/devicetree/bindings/display/renesas,du.txt | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Laurent Pinchart May 11, 2019, 6:23 p.m. UTC | #1
Hi Jacopo,

Thank you for the patch.

On Wed, May 08, 2019 at 07:34:21PM +0200, Jacopo Mondi wrote:
> Document the newly added 'cmms' property which accepts a list of phandle
> and channel index pairs that point to the CMM units available for each
> Display Unit output video channel.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  Documentation/devicetree/bindings/display/renesas,du.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt
> index aedb22b4d161..2ccf78bf9a18 100644
> --- a/Documentation/devicetree/bindings/display/renesas,du.txt
> +++ b/Documentation/devicetree/bindings/display/renesas,du.txt
> @@ -44,6 +44,10 @@ Required Properties:
>      instance that serves the DU channel, and the channel index identifies the
>      LIF instance in that VSP.
>  
> +  - cmms: A list of phandle and channel index tuples to the CMM modules
> +    connected to DU channels. The phandle identifies the CMM instance that
> +    serves the DU channel identified by the index.

Do we need the index ?

It should also be noted that the property is optional for SoCs that
don't have any CMM.

> +
>  Required nodes:
>  
>  The connections to the DU output video ports are modeled using the OF graph

Could you update the example ?
Jacopo Mondi May 15, 2019, 2:12 p.m. UTC | #2
Hi Laurent,
   thanks for the comments

On Sat, May 11, 2019 at 09:23:30PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, May 08, 2019 at 07:34:21PM +0200, Jacopo Mondi wrote:
> > Document the newly added 'cmms' property which accepts a list of phandle
> > and channel index pairs that point to the CMM units available for each
> > Display Unit output video channel.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  Documentation/devicetree/bindings/display/renesas,du.txt | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt
> > index aedb22b4d161..2ccf78bf9a18 100644
> > --- a/Documentation/devicetree/bindings/display/renesas,du.txt
> > +++ b/Documentation/devicetree/bindings/display/renesas,du.txt
> > @@ -44,6 +44,10 @@ Required Properties:
> >      instance that serves the DU channel, and the channel index identifies the
> >      LIF instance in that VSP.
> >
> > +  - cmms: A list of phandle and channel index tuples to the CMM modules
> > +    connected to DU channels. The phandle identifies the CMM instance that
> > +    serves the DU channel identified by the index.
>
> Do we need the index ?
>

Well, I struggled a bit at deciding if this was a good idea or not.
In the end I decided to use the index, as in this version, by just
providing the cmm phandle, the CMM gets enabled for the DU channel it
is associated to. It is true I could just enumerate them, and assign
the CMM corresponding to the first phandle to channel #0, the second
to channel #1 and so on, but in the (very unlikely?) case where a
developer what to enable CMM for, say, channel #0 and #2 but not #1,
this scheme would break, as I have then decided to have a mandatory
channel index to make the association stable. True that a CMM unit is
associated to a DU channel only, and I could derive this from the base
address or a custom property (like 'renesa,du-channel) in the CMM
device node, but this seems better handled here.

Now that I wrote this, I wonder if I actually need to know which
channel a CMM is associated to, or I could just go and enable the ones
listed in the 'cmms' property and that's it. Was this the idea behind
your question?

Thanks
   j
> It should also be noted that the property is optional for SoCs that
> don't have any CMM.
>
> > +
> >  Required nodes:
> >
> >  The connections to the DU output video ports are modeled using the OF graph
>
> Could you update the example ?
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart May 16, 2019, 10:40 a.m. UTC | #3
Hi Jacopo,

On Wed, May 15, 2019 at 04:12:20PM +0200, Jacopo Mondi wrote:
> On Sat, May 11, 2019 at 09:23:30PM +0300, Laurent Pinchart wrote:
> > On Wed, May 08, 2019 at 07:34:21PM +0200, Jacopo Mondi wrote:
> >> Document the newly added 'cmms' property which accepts a list of phandle
> >> and channel index pairs that point to the CMM units available for each
> >> Display Unit output video channel.
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >> ---
> >>  Documentation/devicetree/bindings/display/renesas,du.txt | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt
> >> index aedb22b4d161..2ccf78bf9a18 100644
> >> --- a/Documentation/devicetree/bindings/display/renesas,du.txt
> >> +++ b/Documentation/devicetree/bindings/display/renesas,du.txt
> >> @@ -44,6 +44,10 @@ Required Properties:
> >>      instance that serves the DU channel, and the channel index identifies the
> >>      LIF instance in that VSP.
> >>
> >> +  - cmms: A list of phandle and channel index tuples to the CMM modules
> >> +    connected to DU channels. The phandle identifies the CMM instance that
> >> +    serves the DU channel identified by the index.
> >
> > Do we need the index ?
> 
> Well, I struggled a bit at deciding if this was a good idea or not.
> In the end I decided to use the index, as in this version, by just
> providing the cmm phandle, the CMM gets enabled for the DU channel it
> is associated to. It is true I could just enumerate them, and assign
> the CMM corresponding to the first phandle to channel #0, the second
> to channel #1 and so on, but in the (very unlikely?) case where a
> developer what to enable CMM for, say, channel #0 and #2 but not #1,
> this scheme would break, as I have then decided to have a mandatory
> channel index to make the association stable. True that a CMM unit is
> associated to a DU channel only, and I could derive this from the base
> address or a custom property (like 'renesa,du-channel) in the CMM
> device node, but this seems better handled here.

First of all, DT should describe the hardware, enabling or disabling CMM
should not be done through this property. Then, if you really wanted to
control which channels use CMM through DT, you could just insert a 0
value in the cmms array in the corresponding position instead of the
phandler of a CMM node. Finally, you can also use status = "disabled" in
the CMM nodes to disable them, and the DU driver could check that. That
seems a better mechanism if we want to disable CMM support in DT.

> Now that I wrote this, I wonder if I actually need to know which
> channel a CMM is associated to, or I could just go and enable the ones
> listed in the 'cmms' property and that's it. Was this the idea behind
> your question?

You need to know the association, as you will have to configure the
CMMs, not just enable or disable them.

> > It should also be noted that the property is optional for SoCs that
> > don't have any CMM.
> >
> >> +
> >>  Required nodes:
> >>
> >>  The connections to the DU output video ports are modeled using the OF graph
> >
> > Could you update the example ?
> >

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt
index aedb22b4d161..2ccf78bf9a18 100644
--- a/Documentation/devicetree/bindings/display/renesas,du.txt
+++ b/Documentation/devicetree/bindings/display/renesas,du.txt
@@ -44,6 +44,10 @@  Required Properties:
     instance that serves the DU channel, and the channel index identifies the
     LIF instance in that VSP.
 
+  - cmms: A list of phandle and channel index tuples to the CMM modules
+    connected to DU channels. The phandle identifies the CMM instance that
+    serves the DU channel identified by the index.
+
 Required nodes:
 
 The connections to the DU output video ports are modeled using the OF graph