Message-ID: <871rliw9cq.fsf@kurt>
Date:   Sat, 11 Jul 2020 13:59:33 +0200
From:   Kurt Kanzenbach <kurt@...utronix.de>
To:     Rob Herring <robh@...nel.org>,
        Florian Fainelli <f.fainelli@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        netdev <netdev@...r.kernel.org>, devicetree@...r.kernel.org
Subject: Re: [PATCH v1 1/1] dt-bindings: net: dsa: Add DSA yaml binding

Hi,

On Fri Jul 10 2020, Rob Herring wrote:
> On Fri, Jul 10, 2020 at 11:20 AM Florian Fainelli <f.fainelli@...il.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@...il.com>
>> >> Signed-off-by: Kurt Kanzenbach <kurt@...utronix.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?

>
> 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?

>
>> >> +
>> >> +maintainers:
>> >> +  - Andrew Lunn <andrew@...n.ch>
>> >> +  - Florian Fainelli <f.fainelli@...il.com>
>> >> +  - Vivien Didelot <vivien.didelot@...il.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 {
      ...
    }
  }

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

>
>>
>> >
>> >> +          type: object
>> >> +          description: DSA switch ports
>> >> +
>> >> +          allOf:
>> >> +            - $ref: ../ethernet-controller.yaml#
>> >
>> > How does this and 'ethernet' both apply?
>>
>> I think the intent here was to mean that some of the properties from the
>> Ethernet controller such as phy-mode, phy-handle, fixed-link also apply
>> here since the switch port is a simplified Ethernet MAC on a number of
>> counts.
>
> Okay, it's good to explicitly define which of those apply as I imagine
> some don't. Just need "<prop>: true" to do that.

Yes, that was my intent. Only a few properties from the Ethernet
controller are needed. I'll add them like you suggested.

>
> Rob

Thanks,
Kurt

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)

Powered by blists - more mailing lists