| Message ID | 20211224052309.1997096-3-nikita.yoush@cogentembedded.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | add R-Car M3-W+ (r8a99761) LVDS encoder support | expand |
Hi Nikita, Thank you for the patch. On Fri, Dec 24, 2021 at 08:23:08AM +0300, Nikita Yushchenko wrote: > Add the missing lvds0 node for the R-Car M3-W+ SoC. > > Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> > --- > arch/arm64/boot/dts/renesas/r8a77961.dtsi | 27 +++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/arch/arm64/boot/dts/renesas/r8a77961.dtsi b/arch/arm64/boot/dts/renesas/r8a77961.dtsi > index 86d59e7e1a87..a34d5b1d6431 100644 > --- a/arch/arm64/boot/dts/renesas/r8a77961.dtsi > +++ b/arch/arm64/boot/dts/renesas/r8a77961.dtsi > @@ -2718,6 +2718,33 @@ du_out_hdmi0: endpoint { > port@2 { > reg = <2>; > du_out_lvds0: endpoint { > + remote-endpoint = <&lvds0_in>; > + }; > + }; > + }; > + }; > + > + lvds0: lvds@feb90000 { > + compatible = "renesas,r8a77961-lvds"; > + reg = <0 0xfeb90000 0 0x14>; > + clocks = <&cpg CPG_MOD 727>; > + power-domains = <&sysc R8A77961_PD_ALWAYS_ON>; > + resets = <&cpg 727>; > + status = "disabled"; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + lvds0_in: endpoint { > + remote-endpoint = <&du_out_lvds0>; > + }; > + }; > + port@1 { > + reg = <1>; > + lvds0_out: endpoint { > }; Endpoints must have a remote-endpoint property. Let's drop the endpoint here and keep the port only, the endpoint can be declared in board files. If you're fine with this change I can make it when applying the patch. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > }; > };
Hi. Thank you for looking at this. >> + port@1 { >> + reg = <1>; >> + lvds0_out: endpoint { >> }; > > Endpoints must have a remote-endpoint property. Let's drop the endpoint > here and keep the port only, the endpoint can be declared in board > files. > > If you're fine with this change I can make it when applying the patch. This empty endpoint is currently defined in dtsi files for other r-car gen3 SoCs. Goal here is to define lvds0_out label that is then used in extension board dtsi files to link to the port. In this patch I just used the same approach as in files laying nearby. If this approach is not appropriate, then perhaps need to fix it in files for all SoCs, to make it possible for extension board dtsi to be compatible with all of them. Nikita
Hi Nikita, On Wed, Dec 29, 2021 at 08:04:18PM +0300, Nikita Yushchenko wrote: > Hi. > > Thank you for looking at this. > > >> + port@1 { > >> + reg = <1>; > >> + lvds0_out: endpoint { > >> }; > > > > Endpoints must have a remote-endpoint property. Let's drop the endpoint > > here and keep the port only, the endpoint can be declared in board > > files. > > > > If you're fine with this change I can make it when applying the patch. > > This empty endpoint is currently defined in dtsi files for other r-car > gen3 SoCs. > > Goal here is to define lvds0_out label that is then used in extension > board dtsi files to link to the port. > > In this patch I just used the same approach as in files laying nearby. > > If this approach is not appropriate, then perhaps need to fix it in > files for all SoCs, to make it possible for extension board dtsi to be > compatible with all of them. I'm writing a patch to drop those right now :-) I'll CC you.
>> If this approach is not appropriate, then perhaps need to fix it in >> files for all SoCs, to make it possible for extension board dtsi to be >> compatible with all of them. > > I'm writing a patch to drop those right now :-) I'll CC you. Ok. But, are you sure that empty nodes like these are that bad? I was going to suggest a movement in opposite direction - adding more such nodes, so they could form a sort of API for dts plugins describing e.g. displays connectable to boards based on different SoCs. Nikita
Hi Nikita, On Wed, Dec 29, 2021 at 08:19:10PM +0300, Nikita Yushchenko wrote: > >> If this approach is not appropriate, then perhaps need to fix it in > >> files for all SoCs, to make it possible for extension board dtsi to be > >> compatible with all of them. > > > > I'm writing a patch to drop those right now :-) I'll CC you. And of course I hit the send button too fast, sorry :-S https://lore.kernel.org/linux-renesas-soc/20211229191838.27922-1-laurent.pinchart+renesas@ideasonboard.com/T/#t > Ok. > > But, are you sure that empty nodes like these are that bad? > > I was going to suggest a movement in opposite direction - adding more such nodes, so they could form a > sort of API for dts plugins describing e.g. displays connectable to boards based on different SoCs. Endpoints are meant to model a link between two ports, so an endpoint shouldn't exist in isolation. The issue with creating named endpoints in SoC files is that you can't tell there what remote devices may exist, so the endpoint may or may not match the actual hardware design of a board. I think it's better to create endpoints on both sides together in overlays. https://lore.kernel.org/linux-renesas-soc/20211229193135.28767-2-laurent.pinchart+renesas@ideasonboard.com/T/#t
Endpoints are meant to model a link between two ports, so an endpoint > shouldn't exist in isolation. The issue with creating named endpoints in > SoC files is that you can't tell there what remote devices may exist, so > the endpoint may or may not match the actual hardware design of a board. > I think it's better to create endpoints on both sides together in > overlays. > > https://lore.kernel.org/linux-renesas-soc/20211229193135.28767-2-laurent.pinchart+renesas@ideasonboard.com/T/#t What I don't like here is: details of particular SoC (such as "panel gets video from port@1 of &lvds1) leak into per-panel DT fragment. This limits possibilities to share DT fragments between different use cases. In the patch pointed by the above URL, you have to reference both board and panel in the dts file name. I'd prefer to make each DT fragment to use only either entities defined in that fragment itself, or defined "interface entities" between this and "neighbor" DT fragment. Such as: - SoC's DT fragment defines a named port/endpoint to export video stream at - board's DT fragment defines a named panel node corresponding to panel plugged into board's physical connector, and connects endpoints with SoC's video export, - panel's DT fragment extends the panel node from board with video mode information for this particular panel. And similar for backlight, power, and whatever else exposed on the physical panel connector. So for the board's physical connector there is a set of board-DT-provided entities for use by DT fragment of whatever component plugged to the connector, without direct references to final SoC interfaces. And possibility to reuse DT fragments between boards, and probably have a library of DT fragments for hardware currently available in the market, usable with different boards where that hardware can be connected. Nikita
Hi Nikito, On Thu, Dec 30, 2021 at 12:12:04AM +0300, Nikita Yushchenko wrote: > Endpoints are meant to model a link between two ports, so an endpoint > > shouldn't exist in isolation. The issue with creating named endpoints in > > SoC files is that you can't tell there what remote devices may exist, so > > the endpoint may or may not match the actual hardware design of a board. > > I think it's better to create endpoints on both sides together in > > overlays. > > > > https://lore.kernel.org/linux-renesas-soc/20211229193135.28767-2-laurent.pinchart+renesas@ideasonboard.com/T/#t > > What I don't like here is: details of particular SoC (such as "panel gets video from port@1 of &lvds1) > leak into per-panel DT fragment. > > This limits possibilities to share DT fragments between different use cases. In the patch pointed by the > above URL, you have to reference both board and panel in the dts file name. > > I'd prefer to make each DT fragment to use only either entities defined in that fragment itself, or > defined "interface entities" between this and "neighbor" DT fragment. > > Such as: > - SoC's DT fragment defines a named port/endpoint to export video stream at > - board's DT fragment defines a named panel node corresponding to panel plugged into board's physical > connector, and connects endpoints with SoC's video export, > - panel's DT fragment extends the panel node from board with video mode information for this particular > panel. > > And similar for backlight, power, and whatever else exposed on the physical panel connector. > > So for the board's physical connector there is a set of board-DT-provided entities for use by DT > fragment of whatever component plugged to the connector, without direct references to final SoC > interfaces. And possibility to reuse DT fragments between boards, and probably have a library of DT > fragments for hardware currently available in the market, usable with different boards where that > hardware can be connected. I agree it's annoying, but we'll have a similar problem, just the other way around, with an endpoint defined in the SoC dtsi. Many R-Car SoCs have two LVDS encoders, and you can attach a panel to either of them. Some boards use LVDS0, some boards use LVDS1, and some boards could even use both. A real solution for this problem will require a new concept. The "DT connector" proposal is related to this problem space. There's also a proprietary implementation in the Rapsberry Pi boot loader of a mechanism to support parametrized overlays ([2] and [3], or [4] for an example of how a panel reset or backlight GPIO can be parametrized). [1] https://lwn.net/Articles/689783/ [2] https://www.raspberrypi.com/documentation/computers/configuration.html#part3 [3] https://github.com/raspberrypi/firmware/blob/master/boot/overlays/README#L122 [4] https://github.com/raspberrypi/firmware/blob/master/boot/overlays/README#L312
>> I'd prefer to make each DT fragment to use only either entities defined in that fragment itself, or >> defined "interface entities" between this and "neighbor" DT fragment. >> >> Such as: >> - SoC's DT fragment defines a named port/endpoint to export video stream at >> - board's DT fragment defines a named panel node corresponding to panel plugged into board's physical >> connector, and connects endpoints with SoC's video export, >> - panel's DT fragment extends the panel node from board with video mode information for this particular >> panel. >> ... > > I agree it's annoying, but we'll have a similar problem, just the other > way around, with an endpoint defined in the SoC dtsi. Many R-Car SoCs > have two LVDS encoders, and you can attach a panel to either of them. > Some boards use LVDS0, some boards use LVDS1, and some boards could even > use both. The case of "some boards use LVDS0, some boards use LVDS1" is directly addressed by what I'm trying to suggest. The per-board DT fragment can completely hide board's connection details, without a need for any new concept. The case of "some boards could even use both" indeed needs a some way to parametrize panel's DT fragment, and perhaps load two instances of it with different parameters. To some extent this is doable via preprocessor magic and multiple includes, although this approach has obvious disadvantages. > A real solution for this problem will require a new concept. The "DT > connector" proposal is related to this problem space. There's also a > proprietary implementation in the Rapsberry Pi boot loader of a > mechanism to support parametrized overlays ([2] and [3], or [4] for an > example of how a panel reset or backlight GPIO can be parametrized). So the problem is already recognized for years... what stops from wider adoption of this or similar solutions? And - if/while that is not being done - can't we at least try to follow more portable DT coding pattern while staying within existing concepts? Nikita
Hi Nikita, On Thu, Dec 30, 2021 at 08:30:43AM +0300, Nikita Yushchenko wrote: > >> I'd prefer to make each DT fragment to use only either entities defined in that fragment itself, or > >> defined "interface entities" between this and "neighbor" DT fragment. > >> > >> Such as: > >> - SoC's DT fragment defines a named port/endpoint to export video stream at > >> - board's DT fragment defines a named panel node corresponding to panel plugged into board's physical > >> connector, and connects endpoints with SoC's video export, > >> - panel's DT fragment extends the panel node from board with video mode information for this particular > >> panel. > >> ... > > > > I agree it's annoying, but we'll have a similar problem, just the other > > way around, with an endpoint defined in the SoC dtsi. Many R-Car SoCs > > have two LVDS encoders, and you can attach a panel to either of them. > > Some boards use LVDS0, some boards use LVDS1, and some boards could even > > use both. > > The case of "some boards use LVDS0, some boards use LVDS1" is directly addressed by what I'm trying to > suggest. The per-board DT fragment can completely hide board's connection details, without a need for > any new concept. We could do this by adding a label to the port instead of the endpoint in the SoC .dtsi. lvds0: lvds@.... { ... ports { port@0 { lvds0_in: endpoint { remote-endpoint = <&du_out_lvds0>; }; }; lvds_out_panel_port: port@1 { }; }; It would however likely be better do add the label to port@1 in the board .dts though, as usage of a particular LVDS output is a board property, not an SoC property. Then, in the overlay, panel { port { panel_in: endpoint { remote_endpoint <&lvds_out_panel>; }; }; }; &lvds_out_panel_port { lvds_out_panel: endpoint { remote-endpoint = <&panel_in>; }; }; There's one caveat though: The LVDS DT nodes are disabled in the SoC .dtsi, so the overlay would need to have &lvds0 { status = "okay"; }; and that would need to reference the correct lvds node. Without parameterized overlays, I don't think we can solve this issue neatly (especially given that panels will often have control GPIOs, or GPIO-controlled regulators, that could be wired to different SoC GPIOs on different boards). > The case of "some boards could even use both" indeed needs a some way to parametrize panel's DT > fragment, and perhaps load two instances of it with different parameters. To some extent this is doable > via preprocessor magic and multiple includes, although this approach has obvious disadvantages. > > > A real solution for this problem will require a new concept. The "DT > > connector" proposal is related to this problem space. There's also a > > proprietary implementation in the Rapsberry Pi boot loader of a > > mechanism to support parametrized overlays ([2] and [3], or [4] for an > > example of how a panel reset or backlight GPIO can be parametrized). > > So the problem is already recognized for years... what stops from > wider adoption of this or similar solutions? Someone to continue working on it I suppose :-) > And - if/while that is not being done - can't we at least try to > follow more portable DT coding pattern while staying within existing > concepts? We could use a label for the port node as shown above. It's not a complete solution, but it may help. I'm not sure how to solve the enabling of the lvds encoder DT node though.
Hi Laurent, On Wed, Dec 29, 2021 at 5:56 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Fri, Dec 24, 2021 at 08:23:08AM +0300, Nikita Yushchenko wrote: > > Add the missing lvds0 node for the R-Car M3-W+ SoC. > > > > Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> > > --- > > arch/arm64/boot/dts/renesas/r8a77961.dtsi | 27 +++++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/renesas/r8a77961.dtsi b/arch/arm64/boot/dts/renesas/r8a77961.dtsi > > index 86d59e7e1a87..a34d5b1d6431 100644 > > --- a/arch/arm64/boot/dts/renesas/r8a77961.dtsi > > +++ b/arch/arm64/boot/dts/renesas/r8a77961.dtsi > > @@ -2718,6 +2718,33 @@ du_out_hdmi0: endpoint { > > port@2 { > > reg = <2>; > > du_out_lvds0: endpoint { > > + remote-endpoint = <&lvds0_in>; > > + }; > > + }; > > + }; > > + }; > > + > > + lvds0: lvds@feb90000 { > > + compatible = "renesas,r8a77961-lvds"; > > + reg = <0 0xfeb90000 0 0x14>; > > + clocks = <&cpg CPG_MOD 727>; > > + power-domains = <&sysc R8A77961_PD_ALWAYS_ON>; > > + resets = <&cpg 727>; > > + status = "disabled"; > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + port@0 { > > + reg = <0>; > > + lvds0_in: endpoint { > > + remote-endpoint = <&du_out_lvds0>; > > + }; > > + }; > > + port@1 { > > + reg = <1>; > > + lvds0_out: endpoint { > > }; > > Endpoints must have a remote-endpoint property. Let's drop the endpoint > here and keep the port only, the endpoint can be declared in board > files. > > If you're fine with this change I can make it when applying the patch. Isn't this patch for me to apply to renesas-devel? > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks! 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 Mon, Jan 10, 2022 at 09:43:58AM +0100, Geert Uytterhoeven wrote: > On Wed, Dec 29, 2021 at 5:56 PM Laurent Pinchart wrote: > > On Fri, Dec 24, 2021 at 08:23:08AM +0300, Nikita Yushchenko wrote: > > > Add the missing lvds0 node for the R-Car M3-W+ SoC. > > > > > > Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> > > > --- > > > arch/arm64/boot/dts/renesas/r8a77961.dtsi | 27 +++++++++++++++++++++++ > > > 1 file changed, 27 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/renesas/r8a77961.dtsi b/arch/arm64/boot/dts/renesas/r8a77961.dtsi > > > index 86d59e7e1a87..a34d5b1d6431 100644 > > > --- a/arch/arm64/boot/dts/renesas/r8a77961.dtsi > > > +++ b/arch/arm64/boot/dts/renesas/r8a77961.dtsi > > > @@ -2718,6 +2718,33 @@ du_out_hdmi0: endpoint { > > > port@2 { > > > reg = <2>; > > > du_out_lvds0: endpoint { > > > + remote-endpoint = <&lvds0_in>; > > > + }; > > > + }; > > > + }; > > > + }; > > > + > > > + lvds0: lvds@feb90000 { > > > + compatible = "renesas,r8a77961-lvds"; > > > + reg = <0 0xfeb90000 0 0x14>; > > > + clocks = <&cpg CPG_MOD 727>; > > > + power-domains = <&sysc R8A77961_PD_ALWAYS_ON>; > > > + resets = <&cpg 727>; > > > + status = "disabled"; > > > + > > > + ports { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + port@0 { > > > + reg = <0>; > > > + lvds0_in: endpoint { > > > + remote-endpoint = <&du_out_lvds0>; > > > + }; > > > + }; > > > + port@1 { > > > + reg = <1>; > > > + lvds0_out: endpoint { > > > }; > > > > Endpoints must have a remote-endpoint property. Let's drop the endpoint > > here and keep the port only, the endpoint can be declared in board > > files. > > > > If you're fine with this change I can make it when applying the patch. > > Isn't this patch for me to apply to renesas-devel? Even better indeed :-) > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
On Fri, Dec 24, 2021 at 6:23 AM Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote: > Add the missing lvds0 node for the R-Car M3-W+ SoC. > > Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> i.e. will queue in renesas-devel for v5.18. 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
> i.e. will queue in renesas-devel for v5.18.
that is, for current + 2 ?
Hi Nikita, On Mon, Jan 10, 2022 at 10:51 AM Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote: > > i.e. will queue in renesas-devel for v5.18. > > that is, for current + 2 ? That is correct, as the merge window for v5.17 has already opened. The deadline for new features to be accepted for v5.17 in the soc tree was around v5.16-rc6. 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
>>>> + port@1 { >>>> + reg = <1>; >>>> + lvds0_out: endpoint { >>>> }; >>> >>> Endpoints must have a remote-endpoint property. Let's drop the endpoint >>> here and keep the port only, the endpoint can be declared in board >>> files. >>> >>> If you're fine with this change I can make it when applying the patch. >> >> This empty endpoint is currently defined in dtsi files for other r-car >> gen3 SoCs. >> >> Goal here is to define lvds0_out label that is then used in extension >> board dtsi files to link to the port. >> >> In this patch I just used the same approach as in files laying nearby. >> >> If this approach is not appropriate, then perhaps need to fix it in >> files for all SoCs, to make it possible for extension board dtsi to be >> compatible with all of them. > > I'm writing a patch to drop those right now :-) I'll CC you. This is not the only place where rcag-gen3 dtsi files are using empty-endpoint pattern. du rgb port is defined in the same way. And, I've submitted a patch some weeks ago [1] that hooked into that. [1] https://lore.kernel.org/lkml/20211225115308.2152364-1-nikita.yoush@cogentembedded.com/ Since there was no reply, I am about to resubmit it. But, perhaps need to do something with empty-endpoint pattern first? Nikita
Hi Nikita, On Wed, Jan 12, 2022 at 10:10 PM Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote: > > I'm writing a patch to drop those right now :-) I'll CC you. > > This is not the only place where rcag-gen3 dtsi files are using empty-endpoint pattern. > > du rgb port is defined in the same way. > > And, I've submitted a patch some weeks ago [1] that hooked into that. > > [1] https://lore.kernel.org/lkml/20211225115308.2152364-1-nikita.yoush@cogentembedded.com/ > > Since there was no reply, I am about to resubmit it. > But, perhaps need to do something with empty-endpoint pattern first? No need to resend for now, it is still in my review backlog (Hi Xmas/NY ;-). 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
diff --git a/arch/arm64/boot/dts/renesas/r8a77961.dtsi b/arch/arm64/boot/dts/renesas/r8a77961.dtsi index 86d59e7e1a87..a34d5b1d6431 100644 --- a/arch/arm64/boot/dts/renesas/r8a77961.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a77961.dtsi @@ -2718,6 +2718,33 @@ du_out_hdmi0: endpoint { port@2 { reg = <2>; du_out_lvds0: endpoint { + remote-endpoint = <&lvds0_in>; + }; + }; + }; + }; + + lvds0: lvds@feb90000 { + compatible = "renesas,r8a77961-lvds"; + reg = <0 0xfeb90000 0 0x14>; + clocks = <&cpg CPG_MOD 727>; + power-domains = <&sysc R8A77961_PD_ALWAYS_ON>; + resets = <&cpg 727>; + status = "disabled"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + lvds0_in: endpoint { + remote-endpoint = <&du_out_lvds0>; + }; + }; + port@1 { + reg = <1>; + lvds0_out: endpoint { }; }; };
Add the missing lvds0 node for the R-Car M3-W+ SoC. Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> --- arch/arm64/boot/dts/renesas/r8a77961.dtsi | 27 +++++++++++++++++++++++ 1 file changed, 27 insertions(+)