| Message ID | 20200214082623.4893-1-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] dt-bindings: display: renesas: du: Document optional reset properties | expand |
Hi Geert, On 14/02/2020 08:26, Geert Uytterhoeven wrote: > Document the optional properties for describing module resets, to > support resetting display channels on R-Car Gen2 and Gen3. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Acked-by: Rob Herring <robh@kernel.org> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > --- > Who's taking this kind of patches? > V1 was submmitted in March 2017. Hrm ... presumably through the DRM subsystem trees? Or just for Laurent to pick up ... -- KB > > v5: > - Rebase on top of renesas,cmms and renesas,vsps patches, > > v4: > - Use "All but R8A7779" instead of "R8A779[0123456]", to reduce future > churn, > > v3: > - Add Acked-by, > - Drop LVDS resets, as LVDS is now covered by a separate binding, > - Update the example. > > v2: > - s/phandles/phandle/. > --- > .../devicetree/bindings/display/renesas,du.txt | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt > index eb4ae41fe41f83c7..51cd4d1627703a15 100644 > --- a/Documentation/devicetree/bindings/display/renesas,du.txt > +++ b/Documentation/devicetree/bindings/display/renesas,du.txt > @@ -50,6 +50,14 @@ Required Properties: > VSP instance that serves the DU channel, and the channel index identifies > the LIF instance in that VSP. > > +Optional properties: > + - resets: A list of phandle + reset-specifier pairs, one for each entry in > + the reset-names property. > + - reset-names: Names of the resets. This property is model-dependent. > + - All but R8A7779 use one reset for a group of one or more successive > + channels. The resets must be named "du.x" with "x" being the numerical > + index of the lowest channel in the group. > + > Required nodes: > > The connections to the DU output video ports are modeled using the OF graph > @@ -96,6 +104,8 @@ Example: R8A7795 (R-Car H3) ES2.0 DU > <&cpg CPG_MOD 722>, > <&cpg CPG_MOD 721>; > clock-names = "du.0", "du.1", "du.2", "du.3"; > + resets = <&cpg 724>, <&cpg 722>; > + reset-names = "du.0", "du.2"; > renesas,cmms = <&cmm0>, <&cmm1>, <&cmm2>, <&cmm3>; > renesas,vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>; > >
Hi Geert, On Fri, Feb 14, 2020 at 09:26:23AM +0100, Geert Uytterhoeven wrote: > Document the optional properties for describing module resets, to > support resetting display channels on R-Car Gen2 and Gen3. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Acked-by: Rob Herring <robh@kernel.org> > --- > Who's taking this kind of patches? > V1 was submmitted in March 2017. My bad. > > v5: > - Rebase on top of renesas,cmms and renesas,vsps patches, > > v4: > - Use "All but R8A7779" instead of "R8A779[0123456]", to reduce future > churn, > > v3: > - Add Acked-by, > - Drop LVDS resets, as LVDS is now covered by a separate binding, > - Update the example. > > v2: > - s/phandles/phandle/. > --- > .../devicetree/bindings/display/renesas,du.txt | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt > index eb4ae41fe41f83c7..51cd4d1627703a15 100644 > --- a/Documentation/devicetree/bindings/display/renesas,du.txt > +++ b/Documentation/devicetree/bindings/display/renesas,du.txt > @@ -50,6 +50,14 @@ Required Properties: > VSP instance that serves the DU channel, and the channel index identifies > the LIF instance in that VSP. > > +Optional properties: > + - resets: A list of phandle + reset-specifier pairs, one for each entry in > + the reset-names property. > + - reset-names: Names of the resets. This property is model-dependent. > + - All but R8A7779 use one reset for a group of one or more successive > + channels. The resets must be named "du.x" with "x" being the numerical > + index of the lowest channel in the group. I've now reviewed the patches that add those properties to our .dtsi files, and I wonder how we should handle the two SoCs that have DU0, DU1 and DU3, but not DU2. The reset resource is tied to a group of two channels, so we would use du.0 and du.2 respectively, but that conflicts with the above text. I'm trying to think about the implementation on the driver side, where group resources are associated with a group object, whose index is computed by dividing the channel number by 2. We could have a special case in group initialization that uses du.3 instead of du.2 for the second group. What do you think ? Probably overkill, and we should go for du.3 ? > + > Required nodes: > > The connections to the DU output video ports are modeled using the OF graph > @@ -96,6 +104,8 @@ Example: R8A7795 (R-Car H3) ES2.0 DU > <&cpg CPG_MOD 722>, > <&cpg CPG_MOD 721>; > clock-names = "du.0", "du.1", "du.2", "du.3"; > + resets = <&cpg 724>, <&cpg 722>; > + reset-names = "du.0", "du.2"; > renesas,cmms = <&cmm0>, <&cmm1>, <&cmm2>, <&cmm3>; > renesas,vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>; >
Hi Laurent, On Wed, Feb 19, 2020 at 5:04 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Fri, Feb 14, 2020 at 09:26:23AM +0100, Geert Uytterhoeven wrote: > > Document the optional properties for describing module resets, to > > support resetting display channels on R-Car Gen2 and Gen3. > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Acked-by: Rob Herring <robh@kernel.org> > > --- a/Documentation/devicetree/bindings/display/renesas,du.txt > > +++ b/Documentation/devicetree/bindings/display/renesas,du.txt > > @@ -50,6 +50,14 @@ Required Properties: > > VSP instance that serves the DU channel, and the channel index identifies > > the LIF instance in that VSP. > > > > +Optional properties: > > + - resets: A list of phandle + reset-specifier pairs, one for each entry in > > + the reset-names property. > > + - reset-names: Names of the resets. This property is model-dependent. > > + - All but R8A7779 use one reset for a group of one or more successive > > + channels. The resets must be named "du.x" with "x" being the numerical > > + index of the lowest channel in the group. > > I've now reviewed the patches that add those properties to our .dtsi > files, and I wonder how we should handle the two SoCs that have DU0, DU1 > and DU3, but not DU2. The reset resource is tied to a group of two > channels, so we would use du.0 and du.2 respectively, but that conflicts > with the above text. > > I'm trying to think about the implementation on the driver side, where > group resources are associated with a group object, whose index is > computed by dividing the channel number by 2. We could have a special > case in group initialization that uses du.3 instead of du.2 for the > second group. > > What do you think ? Probably overkill, and we should go for du.3 ? The "division by 2" rule is valid for R-Car Gen3, but not for R-Car Gen2, where there is only a single reset for all channels. Originally we had "du.0-1" and "du.2-3" (hmm, somehow I missed adding this to the changelog for the bindings, but it is present in the changelog for the DTS files), but after switching to "du.0" and "du.2", I always envisioned implementing this by finding a "du.x" reset by looping from the current channel index to 0. That algorithm works for all supported SoCs (irrespective of naming the second reset on R-Car H3-N and M3-N "du.2" or "du.3" ;-) As per your comment about single resets, we could drop reset-names on R-Car Gen2, but doing so would mean another special case in the driver. 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
Hi Geert, On Wed, Feb 19, 2020 at 05:36:57PM +0100, Geert Uytterhoeven wrote: > On Wed, Feb 19, 2020 at 5:04 PM Laurent Pinchart wrote: > > On Fri, Feb 14, 2020 at 09:26:23AM +0100, Geert Uytterhoeven wrote: > > > Document the optional properties for describing module resets, to > > > support resetting display channels on R-Car Gen2 and Gen3. > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Acked-by: Rob Herring <robh@kernel.org> > > > > --- a/Documentation/devicetree/bindings/display/renesas,du.txt > > > +++ b/Documentation/devicetree/bindings/display/renesas,du.txt > > > @@ -50,6 +50,14 @@ Required Properties: > > > VSP instance that serves the DU channel, and the channel index identifies > > > the LIF instance in that VSP. > > > > > > +Optional properties: > > > + - resets: A list of phandle + reset-specifier pairs, one for each entry in > > > + the reset-names property. > > > + - reset-names: Names of the resets. This property is model-dependent. > > > + - All but R8A7779 use one reset for a group of one or more successive > > > + channels. The resets must be named "du.x" with "x" being the numerical > > > + index of the lowest channel in the group. > > > > I've now reviewed the patches that add those properties to our .dtsi > > files, and I wonder how we should handle the two SoCs that have DU0, DU1 > > and DU3, but not DU2. The reset resource is tied to a group of two > > channels, so we would use du.0 and du.2 respectively, but that conflicts > > with the above text. > > > > I'm trying to think about the implementation on the driver side, where > > group resources are associated with a group object, whose index is > > computed by dividing the channel number by 2. We could have a special > > case in group initialization that uses du.3 instead of du.2 for the > > second group. > > > > What do you think ? Probably overkill, and we should go for du.3 ? > > The "division by 2" rule is valid for R-Car Gen3, but not for R-Car > Gen2, where there is only a single reset for all channels. > > Originally we had "du.0-1" and "du.2-3" (hmm, somehow I missed adding > this to the changelog for the bindings, but it is present in the > changelog for the DTS files), but after switching to "du.0" and "du.2", > I always envisioned implementing this by finding a "du.x" reset by > looping from the current channel index to 0. That algorithm works for all > supported SoCs (irrespective of naming the second reset on R-Car H3-N > and M3-N "du.2" or "du.3" ;-) > > As per your comment about single resets, we could drop reset-names on > R-Car Gen2, but doing so would mean another special case in the driver. Probably not worth it indeed. We can handle all this in the driver, let's keep it as-is.
diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt index eb4ae41fe41f83c7..51cd4d1627703a15 100644 --- a/Documentation/devicetree/bindings/display/renesas,du.txt +++ b/Documentation/devicetree/bindings/display/renesas,du.txt @@ -50,6 +50,14 @@ Required Properties: VSP instance that serves the DU channel, and the channel index identifies the LIF instance in that VSP. +Optional properties: + - resets: A list of phandle + reset-specifier pairs, one for each entry in + the reset-names property. + - reset-names: Names of the resets. This property is model-dependent. + - All but R8A7779 use one reset for a group of one or more successive + channels. The resets must be named "du.x" with "x" being the numerical + index of the lowest channel in the group. + Required nodes: The connections to the DU output video ports are modeled using the OF graph @@ -96,6 +104,8 @@ Example: R8A7795 (R-Car H3) ES2.0 DU <&cpg CPG_MOD 722>, <&cpg CPG_MOD 721>; clock-names = "du.0", "du.1", "du.2", "du.3"; + resets = <&cpg 724>, <&cpg 722>; + reset-names = "du.0", "du.2"; renesas,cmms = <&cmm0>, <&cmm1>, <&cmm2>, <&cmm3>; renesas,vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>;