On Mon, Dec 09, 2024 at 04:35:36PM +0100, Niklas Söderlund wrote:
> Hi Rob,
>
> Sorry to bother you but I wonder if you could help me understand why the
> dtc checker warns for the issues I tried to work around in this patch,
> and if possible how I can improve my solution to get rid of the
> warnings.
>
> This patch addresses the same problem for a few different devices. I
> will focus on the last one (/soc/isp@fed00000/ports/port@0) for my
> question, but all warnings here have the same issue.
>
> I have a port node the represents a sink for a video input. This sink
> can either be connected to source A or source B, but not both at the
> same time. So each possible source is represented by an endpoint in the
> port node. Each endpoint have specific register address on the port bus
> that is described in the bindings as they map to different physical pins
> on the hardware.
>
> The issue I have is that not all hardware configurations have both
> source A and B populated. All combinations of A, B and C are possible
> depending on the platform.
>
> A)
> ports {
> ...
> port@0 {
> ...
> sourceA: endpoint@0 {
> reg = <0>
> };
> sourceB: endpoint@1 {
> reg = <1>
> };
> };
> };
>
> B)
> ports {
> ...
> port@0 {
> ...
> sourceA: endpoint@0 {
> reg = <0>
> };
> };
> };
>
> C)
> ports {
> ...
> port@0 {
> ...
> sourceB: endpoint@1 {
> reg = <1>
> };
> };
> };
>
> For option A and C the checker is happy, but for option B the warnings
> this patch tries to address are triggered. While reading the
> dtc/checks.c I find check_graph_child_address() is the one that is
> triggering the warning. And this function explicitly checks for port
> buses with a single endpoint with a register value of 0.
>
> This check was added way back in 2018 in commit df536831d02c ("checks:
> add graph binding checks"). But I can't find any information on the
> specifics. Is this design a bad idea for port buses for some reason I
> don't understand? AFIU this design is possible on other type of buses?
> And do you have any guidance on how I can dig myself out of this hole?
Don't.
The check is only with W=1. It is for cases where there is never more
than 1 endpoint. dtc can't distinguish when that is the case, so there's
going to be cases to ignore. Perhaps we could demote it W=2, but I'd
prefer not to. Making W=1 warning free may be a platform goal, but
that's not an overall go. If we fix something everywhere, then the check
is promoted.
Rob
>
> Thanks for your help.
>
> On 2024-10-16 15:48:19 +0200, Niklas Söderlund wrote:
> > The bindings for the R-Car video capture pipeline uses ports and
> > endpoints to describe which IP is wired up and present on the different
> > SoCs. It is needed to describe both which instance of an IP is
> > connected, and to which port. The bindings try to be as reusable as
> > possible across the different R-Car generations.
> >
> > For example R-Car VIN IP bindings have three ports, where two of them
> > can have multiple endpoints. Not all ports or endpoints are physically
> > present on each generation and/or model of R-Car SoCs.
> >
> > The users of the VIN bindings needs to know not only that a port have
> > one, or more, endpoints but also which particular hardware instance it
> > is. The bindings defines endpoint indexes to correspond to particular
> > hardware instances that can be routed to a port to describe this.
> >
> > This design leads to warnings when compiling the DTB if a port that can
> > describe more then one endpoint only describes a single endpoint. And
> > that endpoint corresponds to be the hardware the bindings defined to
> > index 0. For example compiling R-Car V4H which includes r8a779g0.dtsi,
> >
> > ../r8a779g0.dtsi:1200.12-1210.7: Warning (graph_child_address): /soc/video@e6ef0000/ports/port@2: graph node has single child node 'endpoint@0', #address-cells/#size-cells are not necessary
> > ../r8a779g0.dtsi:1228.12-1238.7: Warning (graph_child_address): /soc/video@e6ef1000/ports/port@2: graph node has single child node 'endpoint@0', #address-cells/#size-cells are not necessary
> > ../r8a779g0.dtsi:1256.12-1266.7: Warning (graph_child_address): /soc/video@e6ef2000/ports/port@2: graph node has single child node 'endpoint@0', #address-cells/#size-cells are not necessary
> > ../r8a779g0.dtsi:1284.12-1294.7: Warning (graph_child_address): /soc/video@e6ef3000/ports/port@2: graph node has single child node 'endpoint@0', #address-cells/#size-cells are not necessary
> > ../r8a779g0.dtsi:1312.12-1322.7: Warning (graph_child_address): /soc/video@e6ef4000/ports/port@2: graph node has single child node 'endpoint@0', #address-cells/#size-cells are not necessary
> > ../r8a779g0.dtsi:1340.12-1350.7: Warning (graph_child_address): /soc/video@e6ef5000/ports/port@2: graph node has single child node 'endpoint@0', #address-cells/#size-cells are not necessary
> > ../r8a779g0.dtsi:1368.12-1378.7: Warning (graph_child_address): /soc/video@e6ef6000/ports/port@2: graph node has single child node 'endpoint@0', #address-cells/#size-cells are not necessary
> > ../r8a779g0.dtsi:1396.12-1406.7: Warning (graph_child_address): /soc/video@e6ef7000/ports/port@2: graph node has single child node 'endpoint@0', #address-cells/#size-cells are not necessary
> > ../r8a779g0.dtsi:2076.12-2086.7: Warning (graph_child_address): /soc/isp@fed00000/ports/port@0: graph node has single child node 'endpoint@0', #address-cells/#size-cells are
> >
> > To avoid these warnings define all possible endpoints for each port in
> > the video capture pipeline, but only set the remote-endpoint property if
> > there is hardware present. This takes care of the warnings, but it also
> > adds empty endpoints that are not connected to anything on that
> > particular SoC.
> >
> > Suggested-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > ---
> > Hi Geert,
> >
> > This only addresses the warnings on V4H. More boards do trigger these
> > warnings but before I address them I thought it was a good idea we
> > agreed if this is a good way forward.
> >
> > In this design I have defined every possible endpoint for the ports
> > involved. This might be a bit excessive as we define endpoints that are
> > not physically possible for V4H. For example V4H only have 2 CSISP
> > instances, while the bindings allow for up-to 4 CSISP as that is
> > possible on V3U which the CSISP bindings are shared with.
> >
> > I'm not sure where to best draw the line. Only adding empty endpoints if
> > they are possible on the SoC sounds good, but what if we get a board
> > with only a single CSISP for example? That would be a single endpoint
> > with an index of 0, this triggering the warning.
> >
> > Maybe do the minimum and only define an extra endpoint for ports that
> > trigger the warning? And if it nots pysically possible for that SoC add
> > a comment? This feels wrong however.
> >
> > Let me know what you think. But it would be nice to get rid of these
> > warnings one way or another.
> >
> > Kind Regards,
> > Niklas
> > ---
> > arch/arm64/boot/dts/renesas/r8a779g0.dtsi | 200 ++++++++++++++++++++++
> > 1 file changed, 200 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/renesas/r8a779g0.dtsi b/arch/arm64/boot/dts/renesas/r8a779g0.dtsi
> > index 61c6b8022ffd..e3079562fe65 100644
> > --- a/arch/arm64/boot/dts/renesas/r8a779g0.dtsi
> > +++ b/arch/arm64/boot/dts/renesas/r8a779g0.dtsi
> > @@ -1364,6 +1364,18 @@ vin00isp0: endpoint@0 {
> > reg = <0>;
> > remote-endpoint = <&isp0vin00>;
> > };
> > +
> > + vin00isp1: endpoint@1 {
> > + reg = <1>;
> > + };
> > +
> > + vin00isp2: endpoint@2 {
> > + reg = <2>;
> > + };
> > +
> > + vin00isp3: endpoint@3 {
> > + reg = <3>;
> > + };
> > };
> > };
> > };
> > @@ -1393,6 +1405,18 @@ vin01isp0: endpoint@0 {
> > reg = <0>;
> > remote-endpoint = <&isp0vin01>;
> > };
> > +
> > + vin01isp1: endpoint@1 {
> > + reg = <1>;
> > + };
> > +
> > + vin01isp2: endpoint@2 {
> > + reg = <2>;
> > + };
> > +
> > + vin01isp3: endpoint@3 {
> > + reg = <3>;
> > + };
> > };
> > };
> > };
> > @@ -1422,6 +1446,18 @@ vin02isp0: endpoint@0 {
> > reg = <0>;
> > remote-endpoint = <&isp0vin02>;
> > };
> > +
> > + vin02isp1: endpoint@1 {
> > + reg = <1>;
> > + };
> > +
> > + vin02isp2: endpoint@2 {
> > + reg = <2>;
> > + };
> > +
> > + vin02isp3: endpoint@3 {
> > + reg = <3>;
> > + };
> > };
> > };
> > };
> > @@ -1451,6 +1487,18 @@ vin03isp0: endpoint@0 {
> > reg = <0>;
> > remote-endpoint = <&isp0vin03>;
> > };
> > +
> > + vin03isp1: endpoint@1 {
> > + reg = <1>;
> > + };
> > +
> > + vin03isp2: endpoint@2 {
> > + reg = <2>;
> > + };
> > +
> > + vin03isp3: endpoint@3 {
> > + reg = <3>;
> > + };
> > };
> > };
> > };
> > @@ -1480,6 +1528,18 @@ vin04isp0: endpoint@0 {
> > reg = <0>;
> > remote-endpoint = <&isp0vin04>;
> > };
> > +
> > + vin04isp1: endpoint@1 {
> > + reg = <1>;
> > + };
> > +
> > + vin04isp2: endpoint@2 {
> > + reg = <2>;
> > + };
> > +
> > + vin04isp3: endpoint@3 {
> > + reg = <3>;
> > + };
> > };
> > };
> > };
> > @@ -1509,6 +1569,18 @@ vin05isp0: endpoint@0 {
> > reg = <0>;
> > remote-endpoint = <&isp0vin05>;
> > };
> > +
> > + vin05isp1: endpoint@1 {
> > + reg = <1>;
> > + };
> > +
> > + vin05isp2: endpoint@2 {
> > + reg = <2>;
> > + };
> > +
> > + vin05isp3: endpoint@3 {
> > + reg = <3>;
> > + };
> > };
> > };
> > };
> > @@ -1538,6 +1610,18 @@ vin06isp0: endpoint@0 {
> > reg = <0>;
> > remote-endpoint = <&isp0vin06>;
> > };
> > +
> > + vin06isp1: endpoint@1 {
> > + reg = <1>;
> > + };
> > +
> > + vin06isp2: endpoint@2 {
> > + reg = <2>;
> > + };
> > +
> > + vin06isp3: endpoint@3 {
> > + reg = <3>;
> > + };
> > };
> > };
> > };
> > @@ -1567,6 +1651,18 @@ vin07isp0: endpoint@0 {
> > reg = <0>;
> > remote-endpoint = <&isp0vin07>;
> > };
> > +
> > + vin07isp1: endpoint@1 {
> > + reg = <1>;
> > + };
> > +
> > + vin07isp2: endpoint@2 {
> > + reg = <2>;
> > + };
> > +
> > + vin07isp3: endpoint@3 {
> > + reg = <3>;
> > + };
> > };
> > };
> > };
> > @@ -1592,10 +1688,22 @@ port@2 {
> >
> > reg = <2>;
> >
> > + vin08isp0: endpoint@0 {
> > + reg = <0>;
> > + };
> > +
> > vin08isp1: endpoint@1 {
> > reg = <1>;
> > remote-endpoint = <&isp1vin08>;
> > };
> > +
> > + vin08isp2: endpoint@2 {
> > + reg = <2>;
> > + };
> > +
> > + vin08isp3: endpoint@3 {
> > + reg = <3>;
> > + };
> > };
> > };
> > };
> > @@ -1621,10 +1729,22 @@ port@2 {
> >
> > reg = <2>;
> >
> > + vin09isp0: endpoint@0 {
> > + reg = <0>;
> > + };
> > +
> > vin09isp1: endpoint@1 {
> > reg = <1>;
> > remote-endpoint = <&isp1vin09>;
> > };
> > +
> > + vin09isp2: endpoint@2 {
> > + reg = <2>;
> > + };
> > +
> > + vin09isp3: endpoint@3 {
> > + reg = <3>;
> > + };
> > };
> > };
> > };
> > @@ -1650,10 +1770,22 @@ port@2 {
> >
> > reg = <2>;
> >
> > + vin10isp0: endpoint@0 {
> > + reg = <0>;
> > + };
> > +
> > vin10isp1: endpoint@1 {
> > reg = <1>;
> > remote-endpoint = <&isp1vin10>;
> > };
> > +
> > + vin10isp2: endpoint@2 {
> > + reg = <2>;
> > + };
> > +
> > + vin10isp3: endpoint@3 {
> > + reg = <3>;
> > + };
> > };
> > };
> > };
> > @@ -1679,10 +1811,22 @@ port@2 {
> >
> > reg = <2>;
> >
> > + vin11isp0: endpoint@0 {
> > + reg = <0>;
> > + };
> > +
> > vin11isp1: endpoint@1 {
> > reg = <1>;
> > remote-endpoint = <&isp1vin11>;
> > };
> > +
> > + vin11isp2: endpoint@2 {
> > + reg = <2>;
> > + };
> > +
> > + vin11isp3: endpoint@3 {
> > + reg = <3>;
> > + };
> > };
> > };
> > };
> > @@ -1708,10 +1852,22 @@ port@2 {
> >
> > reg = <2>;
> >
> > + vin12isp0: endpoint@0 {
> > + reg = <0>;
> > + };
> > +
> > vin12isp1: endpoint@1 {
> > reg = <1>;
> > remote-endpoint = <&isp1vin12>;
> > };
> > +
> > + vin12isp2: endpoint@2 {
> > + reg = <2>;
> > + };
> > +
> > + vin12isp3: endpoint@3 {
> > + reg = <3>;
> > + };
> > };
> > };
> > };
> > @@ -1737,10 +1893,22 @@ port@2 {
> >
> > reg = <2>;
> >
> > + vin13isp0: endpoint@0 {
> > + reg = <0>;
> > + };
> > +
> > vin13isp1: endpoint@1 {
> > reg = <1>;
> > remote-endpoint = <&isp1vin13>;
> > };
> > +
> > + vin13isp2: endpoint@2 {
> > + reg = <2>;
> > + };
> > +
> > + vin13isp3: endpoint@3 {
> > + reg = <3>;
> > + };
> > };
> > };
> > };
> > @@ -1766,10 +1934,22 @@ port@2 {
> >
> > reg = <2>;
> >
> > + vin14isp0: endpoint@0 {
> > + reg = <0>;
> > + };
> > +
> > vin14isp1: endpoint@1 {
> > reg = <1>;
> > remote-endpoint = <&isp1vin14>;
> > };
> > +
> > + vin14isp2: endpoint@2 {
> > + reg = <2>;
> > + };
> > +
> > + vin14isp3: endpoint@3 {
> > + reg = <3>;
> > + };
> > };
> > };
> > };
> > @@ -1795,10 +1975,22 @@ port@2 {
> >
> > reg = <2>;
> >
> > + vin15isp0: endpoint@0 {
> > + reg = <0>;
> > + };
> > +
> > vin15isp1: endpoint@1 {
> > reg = <1>;
> > remote-endpoint = <&isp1vin15>;
> > };
> > +
> > + vin15isp2: endpoint@2 {
> > + reg = <2>;
> > + };
> > +
> > + vin15isp3: endpoint@3 {
> > + reg = <3>;
> > + };
> > };
> > };
> > };
> > @@ -2251,6 +2443,10 @@ isp0csi40: endpoint@0 {
> > reg = <0>;
> > remote-endpoint = <&csi40isp0>;
> > };
> > +
> > + isp0csi41: endpoint@1 {
> > + reg = <1>;
> > + };
> > };
> >
> > port@1 {
> > @@ -2331,6 +2527,10 @@ port@0 {
> >
> > reg = <0>;
> >
> > + isp1csi40: endpoint@0 {
> > + reg = <0>;
> > + };
> > +
> > isp1csi41: endpoint@1 {
> > reg = <1>;
> > remote-endpoint = <&csi41isp1>;
> > --
> > 2.46.2
> >
>
> --
> Kind Regards,
> Niklas Söderlund
