Re: [PATCH v1 1/1] dt-bindings: net: dsa: Add DSA yaml binding

[Rob Herring <robh@kernel.org>]

On Sat, Jul 11, 2020 at 5:59 AM Kurt Kanzenbach <kurt@linutronix.de> wrote:
>
> Hi,
>
> On Fri Jul 10 2020, Rob Herring wrote:
> > On Fri, Jul 10, 2020 at 11:20 AM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>
> >>
> >>
> >> On 7/10/2020 9:45 AM, Rob Herring wrote:
> >> > On Fri, Jul 10, 2020 at 11:06:18AM +0200, Kurt Kanzenbach wrote:
> >> >> For future DSA drivers it makes sense to add a generic DSA yaml binding which
> >> >> can be used then. This was created using the properties from dsa.txt. It
> >> >> includes the ports and the dsa,member property.
> >> >>
> >> >> Suggested-by: Florian Fainelli <f.fainelli@gmail.com>
> >> >> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> >> >> ---
> >> >>  .../devicetree/bindings/net/dsa/dsa.yaml      | 80 +++++++++++++++++++
> >> >>  1 file changed, 80 insertions(+)
> >> >>  create mode 100644 Documentation/devicetree/bindings/net/dsa/dsa.yaml
> >> >>
> >> >> diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
> >> >> new file mode 100644
> >> >> index 000000000000..bec257231bf8
> >> >> --- /dev/null
> >> >> +++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
> >> >> @@ -0,0 +1,80 @@
> >> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> >> +%YAML 1.2
> >> >> +---
> >> >> +$id: http://devicetree.org/schemas/net/dsa/dsa.yaml#
> >> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> >> +
> >> >> +title: Distributed Switch Architecture Device Tree Bindings
> >> >
> >> > DSA is a Linuxism, right?
> >>
> >> Not really, it is a Marvell term that describes their proprietary
> >> switching protocol. Since then DSA within Linux expands well beyond just
> >> Marvell switches, so the terms have been blurred a little bit.
> >
> > Either way, sounds like the terminology here should be more general.
>
> How?

I don't know, just call it 'ethernet switch' binding or something.
>
> >
> > Though I missed that this is really just a conversion of dsa.txt which
> > should be removed in this patch. Otherwise, you'll get me re-reviewing
> > the binding.
>
> Yes, it's a conversion of the dsa.txt. I should have stated that more
> clearly. I didn't remove the .txt file, because it's referenced in all
> the different switch bindings such as b53.txt, ksz.txt and so on. How to
> handle that?

Either update them if not many, or make dsa.txt just point to dsa.yaml
as Andrew mentioned. I haven't looked, but seems like this would be a
small number.

Updating all the users to schema is also welcome. :)

> >> >> +
> >> >> +maintainers:
> >> >> +  - Andrew Lunn <andrew@lunn.ch>
> >> >> +  - Florian Fainelli <f.fainelli@gmail.com>
> >> >> +  - Vivien Didelot <vivien.didelot@gmail.com>
> >> >> +
> >> >> +description:
> >> >> +  Switches are true Linux devices and can be probed by any means. Once probed,
> >> >
> >> > Bindings are OS independent.
>
> OK.
>
> >> >
> >> >> +  they register to the DSA framework, passing a node pointer. This node is
> >> >> +  expected to fulfil the following binding, and may contain additional
> >> >> +  properties as required by the device it is embedded within.
> >> >
> >> > Describe what type of h/w should use this binding.
>
> I took the description from the dsa.txt. However, it makes sense to
> adjust that description. Basically all Ethernet switches with a
> dedicated CPU port should use DSA and this binding.
>
> >> >
> >> >> +
> >> >> +properties:
> >> >> +  $nodename:
> >> >> +    pattern: "^switch(@.*)?$"
> >> >> +
> >> >> +  dsa,member:
> >> >> +    minItems: 2
> >> >> +    maxItems: 2
> >> >> +    description:
> >> >> +      A two element list indicates which DSA cluster, and position within the
> >> >> +      cluster a switch takes. <0 0> is cluster 0, switch 0. <0 1> is cluster 0,
> >> >> +      switch 1. <1 0> is cluster 1, switch 0. A switch not part of any cluster
> >> >> +      (single device hanging off a CPU port) must not specify this property
> >> >> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> >> >> +
> >> >> +  ports:
> >> >> +    type: object
> >> >> +    properties:
> >> >> +      '#address-cells':
> >> >> +        const: 1
> >> >> +      '#size-cells':
> >> >> +        const: 0
> >> >> +
> >> >> +    patternProperties:
> >> >> +      "^port@[0-9]+$":
> >> >
> >> > As ports and port are OF graph nodes, it would be better if we
> >> > standardized on a different name for these. I think we've used
> >> > 'ethernet-port' some.
> >>
> >> Yes we did talk about that before, however when the original DSA binding
> >> was introduced about 7 years ago (or maybe more recently, my memory
> >> fails me now), "ports" was chosen as the encapsulating node. We should
> >> be accepting both ethernet-ports and ports.
> >
> > Yes, I'm aware of the history. Back then it was a free-for-all on node
> > names. Now we're trying to be more disciplined. Ideally, we pick
> > something unique to standardize on and fix the dts files to match as
> > long as the node name is generally a don't care for the OS.
> >
> > The schema says only port/ports is allowed,
>
> Yes, it does.
>
> > so at a minimum
> > ethernet-port/ethernet-ports needs to be added here.
>
> Just to be sure. Instead of
>
>   ports {
>     port@1 {
>       ...
>     }
>   }
>
> The following should be possible as well?
>
>   ethernet-ports {
>     port@1 {

Yes, but probably 'ethernet-port@1' here. Or both can be allowed.

>       ...
>     }
>   }
>
> Is there an easy way to add that alternative to the schema? Or does the
> ethernet-ports property has to be defined as well?

You need a pattern like:

patternProperties:
  "^(ethernet-)?ports$":
    ...

You could also make one property a $ref to another, but I prefer the above.

Rob