Date: Mon, 24 Jun 2024 06:59:43 -0500
From: Adam Ford <aford173@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: devicetree@...r.kernel.org, woods.technical@...il.com, 
	aford@...conembedded.com, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Adam Ford <aford@...il.com>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dt-bindings: net: davinci_emac: Convert to yaml version
 from txt

On Mon, Jun 24, 2024 at 12:07 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> On 23/06/2024 19:09, Adam Ford wrote:
> > The davinci_emac is used by several devices which are still maintained,
> > but to make some improvements, it's necessary to convert from txt to yaml.
> >
> > Signed-off-by: Adam Ford <aford173@...il.com>
> >
> > diff --git a/Documentation/devicetree/bindings/net/davinci_emac.txt b/Documentation/devicetree/bindings/net/davinci_emac.txt
> > deleted file mode 100644
> > index 5e3579e72e2d..000000000000
> > --- a/Documentation/devicetree/bindings/net/davinci_emac.txt
> > +++ /dev/null
> > @@ -1,44 +0,0 @@
> > -* Texas Instruments Davinci EMAC
> > -
> > -This file provides information, what the device node
> > -for the davinci_emac interface contains.
> > -
> > -Required properties:
> > -- compatible: "ti,davinci-dm6467-emac", "ti,am3517-emac" or
> > -  "ti,dm816-emac"
> > -- reg: Offset and length of the register set for the device
> > -- ti,davinci-ctrl-reg-offset: offset to control register
> > -- ti,davinci-ctrl-mod-reg-offset: offset to control module register
> > -- ti,davinci-ctrl-ram-offset: offset to control module ram
> > -- ti,davinci-ctrl-ram-size: size of control module ram
> > -- interrupts: interrupt mapping for the davinci emac interrupts sources:
> > -              4 sources: <Receive Threshold Interrupt
> > -                       Receive Interrupt
> > -                       Transmit Interrupt
> > -                       Miscellaneous Interrupt>
> > -
> > -Optional properties:
> > -- phy-handle: See ethernet.txt file in the same directory.
> > -              If absent, davinci_emac driver defaults to 100/FULL.
> > -- ti,davinci-rmii-en: 1 byte, 1 means use RMII
> > -- ti,davinci-no-bd-ram: boolean, does EMAC have BD RAM?
> > -
> > -The MAC address will be determined using the optional properties
> > -defined in ethernet.txt.
> > -
> > -Example (enbw_cmc board):
> > -     eth0: emac@...0000 {
> > -             compatible = "ti,davinci-dm6467-emac";
> > -             reg = <0x220000 0x4000>;
> > -             ti,davinci-ctrl-reg-offset = <0x3000>;
> > -             ti,davinci-ctrl-mod-reg-offset = <0x2000>;
> > -             ti,davinci-ctrl-ram-offset = <0>;
> > -             ti,davinci-ctrl-ram-size = <0x2000>;
> > -             local-mac-address = [ 00 00 00 00 00 00 ];
> > -             interrupts = <33
> > -                             34
> > -                             35
> > -                             36
> > -                             >;
> > -             interrupt-parent = <&intc>;
> > -     };
> > diff --git a/Documentation/devicetree/bindings/net/davinci_emac.yaml b/Documentation/devicetree/bindings/net/davinci_emac.yaml
> > new file mode 100644
> > index 000000000000..4c2640aef8a1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/davinci_emac.yaml
>
> Filename matching compatible format. Missing vendor prefix. Underscores
> are not used in names or compatibles.

Thank you for the review.

Would a proper name be ti,davinci-emac.yaml?

>
>
> > @@ -0,0 +1,111 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/davinci_emac.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Texas Instruments Davici EMAC
> > +
> > +maintainers:
> > +  - Adam Ford <aford@...il.com>
> > +
> > +description:
> > +  Ethernet based on the Programmable Real-Time Unit and Industrial
> > +  Communication Subsystem.
> > +
> > +allOf:
> > +  - $ref: ethernet-controller.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    items:
>
>
> That's just enum, no need for items here.
>
> > +      - enum:
> > +          - ti,davinci-dm6467-emac # da850
> > +          - ti,dm816-emac
> > +          - ti,am3517-emac
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    minItems: 4
>
> You need to list and describe the items.
>
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    items:
> > +      - const: ick
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  local-mac-address: true
>
> Drop
>
> > +  mac-address: true
>
> Drop
>
> You miss top-level $ref to appropriate schema.
>
> > +
> > +  syscon:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: a phandle to the global system controller on
> > +      to enable/disable interrupts
>
> Drop entire property. There was no such property in old binding and
> nothing explains why it was added.

The am3517.dtsi emac node has a syscon, so I didn't want to break it.
I'll take a look to see what the syscon node on the am3517 does.  I
struggle with if statements in yaml, but if it's necessary for the
am3517, can we keep it if I elaborate on it in the commit message?
>
> > +
> > +  ti,davinci-ctrl-reg-offset:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Offset to control register
> > +
> > +  ti,davinci-ctrl-mod-reg-offset:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Offset to control module register
> > +
> > +  ti,davinci-ctrl-ram-offset:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Offset to control module ram
> > +
> > +  ti,davinci-ctrl-ram-size:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Size of control module ram
> > +
> > +  ti,davinci-rmii-en:
> > +    $ref: /schemas/types.yaml#/definitions/uint8
> > +    description:
> > +      RMII enable means use RMII
> > +
> > +  ti,davinci-no-bd-ram:
> > +    type: boolean
> > +    description:
> > +      Enable if EMAC have BD RAM
> > +
> > +additionalProperties: false
>
> Look at example-schema. This goes after required, although anyway should
> be unevaluatedProperties after adding proper $ref.
>
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - ti,davinci-ctrl-reg-offset
> > +  - ti,davinci-ctrl-mod-reg-offset
> > +  - ti,davinci-ctrl-ram-offset
> > +  - ti,davinci-ctrl-ram-size
> > +
> > +examples:
> > +  - |
> > +    eth0: ethernet@...000 {
>
> Drop label.
>
> > +      compatible = "ti,davinci-dm6467-emac";
> > +      reg = <0x220000 0x4000>;
> > +      ti,davinci-ctrl-reg-offset = <0x3000>;
> > +      ti,davinci-ctrl-mod-reg-offset = <0x2000>;
> > +      ti,davinci-ctrl-ram-offset = <0>;
> > +      ti,davinci-ctrl-ram-size = <0x2000>;
> > +      local-mac-address = [ 00 00 00 00 00 00 ];
> > +      interrupts = <33>, <34>, <35>,<36>;
> > +      clocks = <&psc1 5>;
> > +      power-domains = <&psc1 5>;
> > +      status = "disabled";
>
> Drop. It cannot be disabled, otherwise what would be the point of this
> example?

Sorry, I copy-pasted this from the da850.dtsi node.  I'll remove the
label and the status line.
>
>
> Best regards,
> Krzysztof
>

Powered by blists - more mailing lists