If we don't add something along these lines, it is absolutely impossible
to know, for trees with 3 or more switches, which links represent direct
connections and which don't.

I've studied existing mainline device trees, and it seems that the rule
has been respected thus far. I've actually tested such a 3-switch setup
with the Turris MOX.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 Documentation/devicetree/bindings/net/dsa/dsa-port.yaml | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
index 480120469953..307c61aadcbc 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
@@ -31,10 +31,11 @@ properties:
 
   link:
     description:
-      Should be a list of phandles to other switch's DSA port. This
-      port is used as the outgoing port towards the phandle ports. The
-      full routing information must be given, not just the one hop
-      routes to neighbouring switches
+      Should be a list of phandles to other switch's DSA port. This port is
+      used as the outgoing port towards the phandle ports. In case of trees
+      with more than 2 switches, the full routing information must be given.
+      The first element of the list must be the directly connected DSA port
+      of the adjacent switch.
     $ref: /schemas/types.yaml#/definitions/phandle-array
     items:
       maxItems: 1
-- 
2.34.1

On Fri, Sep 13, 2024 at 04:15:05PM +0300, Vladimir Oltean wrote:
> If we don't add something along these lines, it is absolutely impossible
> to know, for trees with 3 or more switches, which links represent direct
> connections and which don't.
> 
> I've studied existing mainline device trees, and it seems that the rule
> has been respected thus far. I've actually tested such a 3-switch setup
> with the Turris MOX.

What about out of tree (so in u-boot or the likes)? Are there other
users that we need to care about?

This doesn't really seem like an ABI change, if this is the established
convention, but feels like a fixes tag and backports to stable etc are
in order to me.

> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  Documentation/devicetree/bindings/net/dsa/dsa-port.yaml | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
> index 480120469953..307c61aadcbc 100644
> --- a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
> @@ -31,10 +31,11 @@ properties:
>  
>    link:
>      description:
> -      Should be a list of phandles to other switch's DSA port. This
> -      port is used as the outgoing port towards the phandle ports. The
> -      full routing information must be given, not just the one hop
> -      routes to neighbouring switches
> +      Should be a list of phandles to other switch's DSA port. This port is
> +      used as the outgoing port towards the phandle ports. In case of trees
> +      with more than 2 switches, the full routing information must be given.
> +      The first element of the list must be the directly connected DSA port
> +      of the adjacent switch.
>      $ref: /schemas/types.yaml#/definitions/phandle-array
>      items:
>        maxItems: 1
> -- 
> 2.34.1
> 

On Fri, Sep 13, 2024 at 06:04:17PM +0100, Conor Dooley wrote:
> On Fri, Sep 13, 2024 at 04:15:05PM +0300, Vladimir Oltean wrote:
> > If we don't add something along these lines, it is absolutely impossible
> > to know, for trees with 3 or more switches, which links represent direct
> > connections and which don't.
> > 
> > I've studied existing mainline device trees, and it seems that the rule
> > has been respected thus far. I've actually tested such a 3-switch setup
> > with the Turris MOX.
> 
> What about out of tree (so in u-boot or the likes)? Are there other
> users that we need to care about?
> 
> This doesn't really seem like an ABI change, if this is the established
> convention, but feels like a fixes tag and backports to stable etc are
> in order to me.

Looking at the next patch, it does not appear to change the behaviour
of existing drivers, it just adds additional information a driver may
choose to use.

As Vladimir says, all existing instances already appear to be in this
order, but that is just happen stance, because it does not matter. So
i would be reluctant to say there is an established convention.

The mv88e6xxx driver does not care about the order of the list, and
this patch series does not appear to change that. So i'm O.K. with the
code changes.

> > -      Should be a list of phandles to other switch's DSA port. This
> > -      port is used as the outgoing port towards the phandle ports. The
> > -      full routing information must be given, not just the one hop
> > -      routes to neighbouring switches
> > +      Should be a list of phandles to other switch's DSA port. This port is
> > +      used as the outgoing port towards the phandle ports. In case of trees
> > +      with more than 2 switches, the full routing information must be given.
> > +      The first element of the list must be the directly connected DSA port
> > +      of the adjacent switch.

I would prefer to weaken this, just to avoid future backwards
compatibility issues. `must` is too strong, because mv88e6xxx does not
care, and there could be DT blobs out there which do not conform to
this. Maybe 'For the SJA1105, the first element ...".

What i don't want is some future developer reading this, assume it is
actually true when it maybe is not, and making use of it in the
mv88e6xxx or the core, breaking backwards compatibility.

	Andrew

Hi Conor, Andrew,

Thanks for taking a look at the patch.

On Fri, Sep 13, 2024 at 07:26:49PM +0200, Andrew Lunn wrote:
> On Fri, Sep 13, 2024 at 06:04:17PM +0100, Conor Dooley wrote:
> > On Fri, Sep 13, 2024 at 04:15:05PM +0300, Vladimir Oltean wrote:
> > > If we don't add something along these lines, it is absolutely impossible
> > > to know, for trees with 3 or more switches, which links represent direct
> > > connections and which don't.
> > > 
> > > I've studied existing mainline device trees, and it seems that the rule
> > > has been respected thus far. I've actually tested such a 3-switch setup
> > > with the Turris MOX.
> > 
> > What about out of tree (so in u-boot or the likes)? Are there other
> > users that we need to care about?

In U-Boot there is only armada-3720-turris-mox.dts which is in sync with
Linux as far as I'm aware. Also, we don't really support cascade ports
in U-Boot DM_DSA yet. So all device trees in U-Boot should be coming
from Linux. I'm not aware of other device tree users, sadly.

> > This doesn't really seem like an ABI change, if this is the established
> > convention, but feels like a fixes tag and backports to stable etc are
> > in order to me.
> 
> Looking at the next patch, it does not appear to change the behaviour
> of existing drivers, it just adds additional information a driver may
> choose to use.
> 
> As Vladimir says, all existing instances already appear to be in this
> order, but that is just happen stance, because it does not matter. So
> i would be reluctant to say there is an established convention.

Yes, indeed, it's not a convention yet. However, it is a convention I
would really like to establish, based on the practice I have seen.

> The mv88e6xxx driver does not care about the order of the list, and
> this patch series does not appear to change that. So i'm O.K. with the
> code changes.
> 
> > > -      Should be a list of phandles to other switch's DSA port. This
> > > -      port is used as the outgoing port towards the phandle ports. The
> > > -      full routing information must be given, not just the one hop
> > > -      routes to neighbouring switches
> > > +      Should be a list of phandles to other switch's DSA port. This port is
> > > +      used as the outgoing port towards the phandle ports. In case of trees
> > > +      with more than 2 switches, the full routing information must be given.
> > > +      The first element of the list must be the directly connected DSA port
> > > +      of the adjacent switch.
> 
> I would prefer to weaken this, just to avoid future backwards
> compatibility issues. `must` is too strong, because mv88e6xxx does not
> care, and there could be DT blobs out there which do not conform to
> this. Maybe 'For the SJA1105, the first element ...".
> 
> What i don't want is some future developer reading this, assume it is
> actually true when it maybe is not, and making use of it in the
> mv88e6xxx or the core, breaking backwards compatibility.

Backward compatibility issues aside, the way dp->link_dp is populated
can _only_ be done based on the assumption that this is true.

I'm afraid that any verb less strong than "must" would be insufficient
for my patch 3/4.

I have unpublished downstream patches which even make all the rest of
the "link = <...>" elements optional. Bottom line, only the direct
connection between ports (first element) represents hardware description.
The other reachable ports (the routing table, practically speaking) can be*
computed based on breadth-first search at DSA probe time. They are
listed in the device tree for "convenience".

*and IMO, also should be. For a 3-switch daisy chain, there are 8 links
to describe, and for a 4-switch daisy chain, there are 22 links, if my
math is correct. I think it's unreasonable to expect that board DT
writers won't make mistakes in listing this amount of elements, rather
than just concentrating on the physically relevant info - the direct
connection.

Baking the assumption proposed here into the binding now makes the BFS
algorithm perfectly implementable, and the binding much more scalable.

Do you have reasonable concerns that there exist device trees which are
written differently than "first 'link' element is the direct connection"?

> I have unpublished downstream patches which even make all the rest of
> the "link = <...>" elements optional. Bottom line, only the direct
> connection between ports (first element) represents hardware description.
> The other reachable ports (the routing table, practically speaking) can be*
> computed based on breadth-first search at DSA probe time. They are
> listed in the device tree for "convenience".

If you have this code, then i would actually go for a new property,
maybe 'direct-link = <...>;', which only lists the direct
relationship. Keep the current property with its current meaning, an
unordered list. If the new property is present, use it to compute the
table. If both sets of properties are present, ensure they result in
the same table, otherwise -EINVAL.

    Andrew