On Mon, Jun 24, 2024 at 12:07 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> 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@xxxxxxxxx>
> >
> > 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@1e20000 {
> > - 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@xxxxxxxxx>
> > +
> > +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@220000 {
>
> 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
>
