[net-next,v11,4/9] dt-bindings: net: convert hisi-femac.txt to YAML

Commit Message

Yang Xiwen via B4 Relay March 9, 2024, 12:26 p.m. UTC

From: Yang Xiwen <[e-mail]>

Convert the old text binding to new YAML.

While at it, make some changes to the binding:
- The version numbers are not documented publicly. The version also does
not change programming interface. Remove it until it's really needed.
- A few clocks are missing in old binding file. Add them to match the real
hardware.

Reviewed-by: Krzysztof Kozlowski <[e-mail]>
Signed-off-by: Yang Xiwen <[e-mail]>
---
 .../bindings/net/hisilicon,hisi-femac.yaml         | 87 ++++++++++++++++++++++
 .../devicetree/bindings/net/hisilicon-femac.txt    | 41 ----------
 2 files changed, 87 insertions(+), 41 deletions(-)

Comments

Yang Xiwen March 9, 2024, 12:30 p.m. UTC | #1

On 3/9/2024 8:26 PM, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <[e-mail]>
>
> Convert the old text binding to new YAML.
>
> While at it, make some changes to the binding:
> - The version numbers are not documented publicly. The version also does
> not change programming interface. Remove it until it's really needed.
> - A few clocks are missing in old binding file. Add them to match the real
> hardware.
>
> Reviewed-by: Krzysztof Kozlowski <[e-mail]>
> Signed-off-by: Yang Xiwen <[e-mail]>
> ---
>   .../bindings/net/hisilicon,hisi-femac.yaml         | 87 ++++++++++++++++++++++
>   .../devicetree/bindings/net/hisilicon-femac.txt    | 41 ----------
>   2 files changed, 87 insertions(+), 41 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/hisilicon,hisi-femac.yaml b/Documentation/devicetree/bindings/net/hisilicon,hisi-femac.yaml
> new file mode 100644
> index 000000000000..3344d3bfefb8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/hisilicon,hisi-femac.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/hisilicon,hisi-femac.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Hisilicon Fast Ethernet MAC controller
> +
> +maintainers:
> +  - Yang Xiwen <[e-mail]>
> +
> +allOf:
> +  - $ref: ethernet-controller.yaml
> +
> +properties:
> +  compatible:
> +    enum:
> +      - hisilicon,hi3516cv300-femac
> +
> +  reg:
> +    items:
> +      - description: The first region is the MAC core register base and size.
> +      - description: The second region is the global MAC control register.
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: MAC main clock
> +      - description: MAC bus interface clock
> +      - description: PHY clock
> +
> +  clock-names:
> +    items:
> +      - const: mac
> +      - const: macif
> +      - const: phy


Still not very correct here. In downstream the core can also have an 
external PHY. The internal phy is also optional. So maybe this clock 
should be optional.


> +
> +  resets:
> +    items:
> +      - description: MAC reset signal
> +      - description: PHY reset signal


Same here


> +
> +  reset-names:
> +    items:
> +      - const: mac
> +      - const: phy
> +
> +  hisilicon,phy-reset-delays-us:
> +    description: PHY reset timing requirement (in micro seconds).
> +      The integrated PHY usually have a special reset timing sequence and must
> +      interact with MAC controller to accomplish the entire reset procedure. So
> +      these properties belong to MAC controller, not PHY.
> +    items:
> +      - description: pre-reset delay for PHY
> +      - description: reset pulse for PHY
> +      - description: post-reset delay for PHY


And here.


> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - resets
> +  - reset-names
> +  - phy-mode
> +  - phy-handle
> +  - hisilicon,phy-reset-delays-us
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    ethernet@10090000 {
> +        compatible = "hisilicon,hi3516cv300-femac";
> +        reg = <0x10090000 0x1000>, <0x10091300 0x200>;
> +        interrupts = <12>;
> +        clocks = <&clk_femac>, <&clk_femacif>, <&clk_fephy>;
> +        clock-names = "mac", "macif", "phy";
> +        resets = <&crg 0xec 0>, <&crg 0xec 3>;
> +        reset-names = "mac", "phy";
> +        mac-address = [00 00 00 00 00 00];
> +        phy-mode = "mii";
> +        phy-handle = <&fephy>;
> +        hisilicon,phy-reset-delays-us = <10000 20000 20000>;
> +    };
> diff --git a/Documentation/devicetree/bindings/net/hisilicon-femac.txt b/Documentation/devicetree/bindings/net/hisilicon-femac.txt
> deleted file mode 100644
> index 5f96976f3cea..000000000000
> --- a/Documentation/devicetree/bindings/net/hisilicon-femac.txt
> +++ /dev/null
> @@ -1,41 +0,0 @@
> -Hisilicon Fast Ethernet MAC controller
> -
> -Required properties:
> -- compatible: should contain one of the following version strings:
> -	* "hisilicon,hisi-femac-v1"
> -	* "hisilicon,hisi-femac-v2"
> -	and the soc string "hisilicon,hi3516cv300-femac".
> -- reg: specifies base physical address(s) and size of the device registers.
> -  The first region is the MAC core register base and size.
> -  The second region is the global MAC control register.
> -- interrupts: should contain the MAC interrupt.
> -- clocks: A phandle to the MAC main clock.
> -- resets: should contain the phandle to the MAC reset signal(required) and
> -	the PHY reset signal(optional).
> -- reset-names: should contain the reset signal name "mac"(required)
> -	and "phy"(optional).
> -- phy-mode: see ethernet.txt [1].
> -- phy-handle: see ethernet.txt [1].
> -- hisilicon,phy-reset-delays-us: triplet of delays if PHY reset signal given.
> -	The 1st cell is reset pre-delay in micro seconds.
> -	The 2nd cell is reset pulse in micro seconds.
> -	The 3rd cell is reset post-delay in micro seconds.
> -
> -The MAC address will be determined using the optional properties
> -defined in ethernet.txt[1].
> -
> -[1] Documentation/devicetree/bindings/net/ethernet.txt
> -
> -Example:
> -	hisi_femac: ethernet@10090000 {
> -		compatible = "hisilicon,hi3516cv300-femac","hisilicon,hisi-femac-v2";
> -		reg = <0x10090000 0x1000>,<0x10091300 0x200>;
> -		interrupts = <12>;
> -		clocks = <&crg HI3518EV200_ETH_CLK>;
> -		resets = <&crg 0xec 0>,<&crg 0xec 3>;
> -		reset-names = "mac","phy";
> -		mac-address = [00 00 00 00 00 00];
> -		phy-mode = "mii";
> -		phy-handle = <&phy0>;
> -		hisilicon,phy-reset-delays-us = <10000 20000 20000>;
> -	};
>

Currently both binding and driver assume internal PHY is always present. 
This should be fixed in the future.

Jakub Kicinski March 11, 2024, 4:03 p.m. UTC | #2

On Sat, 9 Mar 2024 20:30:21 +0800 Yang Xiwen wrote:
> > +  clock-names:
> > +    items:
> > +      - const: mac
> > +      - const: macif
> > +      - const: phy  
> 
> Still not very correct here. In downstream the core can also have an 
> external PHY. The internal phy is also optional. So maybe this clock 
> should be optional.

You are responding to yourself 4 min after posting?
What is the purpose of your comments?

Yang Xiwen March 12, 2024, 12:49 a.m. UTC | #3

On 3/12/2024 12:03 AM, Jakub Kicinski wrote:
> On Sat, 9 Mar 2024 20:30:21 +0800 Yang Xiwen wrote:
>>> +  clock-names:
>>> +    items:
>>> +      - const: mac
>>> +      - const: macif
>>> +      - const: phy
>> Still not very correct here. In downstream the core can also have an
>> external PHY. The internal phy is also optional. So maybe this clock
>> should be optional.
> You are responding to yourself 4 min after posting?
> What is the purpose of your comments?


Just to remind others or myself this can be improved. But i think it's 
ready to be applied. There won't be similar design in mainline soon i think.

Jakub Kicinski March 12, 2024, 1:08 a.m. UTC | #4

On Tue, 12 Mar 2024 08:49:21 +0800 Yang Xiwen wrote:
> >> Still not very correct here. In downstream the core can also have an
> >> external PHY. The internal phy is also optional. So maybe this clock
> >> should be optional.  
> > You are responding to yourself 4 min after posting?
> > What is the purpose of your comments?  
> 
> Just to remind others or myself this can be improved. But i think it's 
> ready to be applied. There won't be similar design in mainline soon i think.

It's a fairly uncommon thing to do, normally such notes should either
be comments in the code or notes in the commit message.

In any case - the merge window for v6.9 has started, we won't be able
to merge these changes for the next 2 weeks :(

Yang Xiwen March 12, 2024, 1:15 a.m. UTC | #5

On 3/12/2024 9:08 AM, Jakub Kicinski wrote:
> On Tue, 12 Mar 2024 08:49:21 +0800 Yang Xiwen wrote:
>>>> Still not very correct here. In downstream the core can also have an
>>>> external PHY. The internal phy is also optional. So maybe this clock
>>>> should be optional.
>>> You are responding to yourself 4 min after posting?
>>> What is the purpose of your comments?
>> Just to remind others or myself this can be improved. But i think it's
>> ready to be applied. There won't be similar design in mainline soon i think.
> It's a fairly uncommon thing to do, normally such notes should either
> be comments in the code or notes in the commit message.


Yeah. Maybe i can try to improve this in the next 6 weeks. But without 
the hardware to test, i can hardly say my implementation is correct or not.


>
> In any case - the merge window for v6.9 has started, we won't be able
> to merge these changes for the next 2 weeks :(


It's sad to wait for another 6 weeks till v6.10. But i'm okay with it, 
since i still have a lot of other patches pending to be applied for 
Hi3798MV200 SoC.

Patch

diff --git a/Documentation/devicetree/bindings/net/hisilicon,hisi-femac.yaml b/Documentation/devicetree/bindings/net/hisilicon,hisi-femac.yaml
new file mode 100644
index 000000000000..3344d3bfefb8
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/hisilicon,hisi-femac.yaml
@@ -0,0 +1,87 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/hisilicon,hisi-femac.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Hisilicon Fast Ethernet MAC controller
+
+maintainers:
+  - Yang Xiwen <[e-mail]>
+
+allOf:
+  - $ref: ethernet-controller.yaml
+
+properties:
+  compatible:
+    enum:
+      - hisilicon,hi3516cv300-femac
+
+  reg:
+    items:
+      - description: The first region is the MAC core register base and size.
+      - description: The second region is the global MAC control register.
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: MAC main clock
+      - description: MAC bus interface clock
+      - description: PHY clock
+
+  clock-names:
+    items:
+      - const: mac
+      - const: macif
+      - const: phy
+
+  resets:
+    items:
+      - description: MAC reset signal
+      - description: PHY reset signal
+
+  reset-names:
+    items:
+      - const: mac
+      - const: phy
+
+  hisilicon,phy-reset-delays-us:
+    description: PHY reset timing requirement (in micro seconds).
+      The integrated PHY usually have a special reset timing sequence and must
+      interact with MAC controller to accomplish the entire reset procedure. So
+      these properties belong to MAC controller, not PHY.
+    items:
+      - description: pre-reset delay for PHY
+      - description: reset pulse for PHY
+      - description: post-reset delay for PHY
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - resets
+  - reset-names
+  - phy-mode
+  - phy-handle
+  - hisilicon,phy-reset-delays-us
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    ethernet@10090000 {
+        compatible = "hisilicon,hi3516cv300-femac";
+        reg = <0x10090000 0x1000>, <0x10091300 0x200>;
+        interrupts = <12>;
+        clocks = <&clk_femac>, <&clk_femacif>, <&clk_fephy>;
+        clock-names = "mac", "macif", "phy";
+        resets = <&crg 0xec 0>, <&crg 0xec 3>;
+        reset-names = "mac", "phy";
+        mac-address = [00 00 00 00 00 00];
+        phy-mode = "mii";
+        phy-handle = <&fephy>;
+        hisilicon,phy-reset-delays-us = <10000 20000 20000>;
+    };
diff --git a/Documentation/devicetree/bindings/net/hisilicon-femac.txt b/Documentation/devicetree/bindings/net/hisilicon-femac.txt
deleted file mode 100644
index 5f96976f3cea..000000000000
--- a/Documentation/devicetree/bindings/net/hisilicon-femac.txt
+++ /dev/null
@@ -1,41 +0,0 @@ 
-Hisilicon Fast Ethernet MAC controller
-
-Required properties:
-- compatible: should contain one of the following version strings:
-	* "hisilicon,hisi-femac-v1"
-	* "hisilicon,hisi-femac-v2"
-	and the soc string "hisilicon,hi3516cv300-femac".
-- reg: specifies base physical address(s) and size of the device registers.
-  The first region is the MAC core register base and size.
-  The second region is the global MAC control register.
-- interrupts: should contain the MAC interrupt.
-- clocks: A phandle to the MAC main clock.
-- resets: should contain the phandle to the MAC reset signal(required) and
-	the PHY reset signal(optional).
-- reset-names: should contain the reset signal name "mac"(required)
-	and "phy"(optional).
-- phy-mode: see ethernet.txt [1].
-- phy-handle: see ethernet.txt [1].
-- hisilicon,phy-reset-delays-us: triplet of delays if PHY reset signal given.
-	The 1st cell is reset pre-delay in micro seconds.
-	The 2nd cell is reset pulse in micro seconds.
-	The 3rd cell is reset post-delay in micro seconds.
-
-The MAC address will be determined using the optional properties
-defined in ethernet.txt[1].
-
-[1] Documentation/devicetree/bindings/net/ethernet.txt
-
-Example:
-	hisi_femac: ethernet@10090000 {
-		compatible = "hisilicon,hi3516cv300-femac","hisilicon,hisi-femac-v2";
-		reg = <0x10090000 0x1000>,<0x10091300 0x200>;
-		interrupts = <12>;
-		clocks = <&crg HI3518EV200_ETH_CLK>;
-		resets = <&crg 0xec 0>,<&crg 0xec 3>;
-		reset-names = "mac","phy";
-		mac-address = [00 00 00 00 00 00];
-		phy-mode = "mii";
-		phy-handle = <&phy0>;
-		hisilicon,phy-reset-delays-us = <10000 20000 20000>;
-	};