| Message ID | 20240913131507.2760966-3-vladimir.oltean@nxp.com |
|---|---|
| State | Under Review |
| Headers | show |
| Series | Cascaded management xmit for SJA1105 DSA driver | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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
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
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(-)