dt-bindings: net: davinci_emac: Convert to yaml version from txt

[PATCH] dt-bindings: net: davinci_emac: Convert to yaml version from txt

Posted by Adam Ford 1 year, 5 months ago

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@gmail.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@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
@@ -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@gmail.com>
+
+description:
+  Ethernet based on the Programmable Real-Time Unit and Industrial
+  Communication Subsystem.
+
+allOf:
+  - $ref: ethernet-controller.yaml#
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - ti,davinci-dm6467-emac # da850
+          - ti,dm816-emac
+          - ti,am3517-emac
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    minItems: 4
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: ick
+
+  power-domains:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  local-mac-address: true
+  mac-address: true
+
+  syscon:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: a phandle to the global system controller on
+      to enable/disable interrupts
+
+  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
+
+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 {
+      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";
+    };
+
-- 
2.43.0

Re: [PATCH] dt-bindings: net: davinci_emac: Convert to yaml version from txt

Posted by Krzysztof Kozlowski 1 year, 5 months ago

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@gmail.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@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.


> @@ -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@gmail.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.

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


Best regards,
Krzysztof

Re: [PATCH] dt-bindings: net: davinci_emac: Convert to yaml version from txt

Posted by Adam Ford 1 year, 5 months ago

On Mon, Jun 24, 2024 at 12:07 AM Krzysztof Kozlowski <krzk@kernel.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@gmail.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@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@gmail.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@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
>

Re: [PATCH] dt-bindings: net: davinci_emac: Convert to yaml version from txt

Posted by Krzysztof Kozlowski 1 year, 5 months ago

On 24/06/2024 13:59, Adam Ford wrote:

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

Yes, it's fine.

> 
>>
>>
>>> @@ -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@gmail.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?

Explain in commit msg changes to the binding done during conversion.
With a rationale why these are needed (e.g. existing DTS and Linux
drivers use them).


Best regards,
Krzysztof