[PATCH net-next] dt-bindings: net: Convert hisilicon,hns-mdio to DT schema

[PATCH net-next] dt-bindings: net: Convert hisilicon,hns-mdio to DT schema

[Rob Herring (Arm)]

Convert the hisilicon,hns-mdio binding to DT schema format. Add the
missing 'subctrl-vbase' property which is already in use.

Assisted-by: LLM
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 .../bindings/net/hisilicon,hns-mdio.yaml      | 59 +++++++++++++++++++
 .../bindings/net/hisilicon-hns-mdio.txt       | 27 ---------
 2 files changed, 59 insertions(+), 27 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/hisilicon,hns-mdio.yaml
 delete mode 100644 Documentation/devicetree/bindings/net/hisilicon-hns-mdio.txt

diff --git a/Documentation/devicetree/bindings/net/hisilicon,hns-mdio.yaml b/Documentation/devicetree/bindings/net/hisilicon,hns-mdio.yaml
new file mode 100644
index 000000000000..b8350794650d
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/hisilicon,hns-mdio.yaml
@@ -0,0 +1,59 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/hisilicon,hns-mdio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Hisilicon HNS MDIO bus controller
+
+maintainers:
+  - Jian Shen <shenjian15@huawei.com>
+  - Jijie Shao <shaojijie@huawei.com>
+
+allOf:
+  - $ref: mdio.yaml#
+
+properties:
+  compatible:
+    enum:
+      - hisilicon,hns-mdio
+      - hisilicon,mdio
+
+  reg:
+    maxItems: 1
+
+  subctrl-vbase:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: syscon phandle
+          - description: MDIO clock enable register offset
+          - description: MDIO reset request register offset
+          - description: MDIO reset done register offset
+          - description: MDIO reset deassert register offset
+
+required:
+  - compatible
+  - reg
+  - '#address-cells'
+  - '#size-cells'
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        mdio@803c0000 {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            compatible = "hisilicon,hns-mdio";
+            reg = <0x0 0x803c0000 0x0 0x10000>;
+
+            ethernet-phy@0 {
+                reg = <0>;
+            };
+        };
+    };
diff --git a/Documentation/devicetree/bindings/net/hisilicon-hns-mdio.txt b/Documentation/devicetree/bindings/net/hisilicon-hns-mdio.txt
deleted file mode 100644
index 4a7ede9657b0..000000000000
--- a/Documentation/devicetree/bindings/net/hisilicon-hns-mdio.txt
+++ /dev/null
@@ -1,27 +0,0 @@
-Hisilicon MDIO bus controller
-
-Properties:
-- compatible: can be one of:
-	"hisilicon,hns-mdio"
-	"hisilicon,mdio"
-  "hisilicon,hns-mdio" is recommended to be used for hip05 and later SOCs,
-  while "hisilicon,mdio" is optional for backwards compatibility only on
-  hip04 Soc.
-- reg: The base address of the MDIO bus controller register bank.
-- #address-cells: Must be <1>.
-- #size-cells: Must be <0>.  MDIO addresses have no size component.
-
-Typically an MDIO bus might have several children.
-
-Example:
-         mdio@803c0000 {
-                   #address-cells = <1>;
-                   #size-cells = <0>;
-                   compatible = "hisilicon,hns-mdio","hisilicon,mdio";
-                   reg = <0x0 0x803c0000 0x0 0x10000>;
-
-                   ethernet-phy@0 {
-                            ...
-                            reg = <0>;
-                   };
-         };
-- 
2.53.0

Re: [PATCH net-next] dt-bindings: net: Convert hisilicon,hns-mdio to DT schema

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In the new
  Documentation/devicetree/bindings/net/hisilicon,hns-mdio.yaml, two of…
- [Low] The new schema lists 'subctrl-vbase' but doesn't make it
  required; the required list is only compatible, reg, #address-cells…

--

dt-bindings: net: Convert hisilicon,hns-mdio to DT schema

This converts the hisilicon,hns-mdio text binding to DT schema format.
It also adds the subctrl-vbase property. The driver and the hip06/hip07
device trees already use that property.

> diff --git a/Documentation/devicetree/bindings/net/hisilicon,hns-mdio.yaml b/Documentation/devicetree/bindings/net/hisilicon,hns-mdio.yaml
> new file mode 100644
> index 0000000000000..b8350794650d4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/hisilicon,hns-mdio.yaml
> @@ -0,0 +1,59 @@

[ ... ]

> +  subctrl-vbase:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - items:
> +          - description: syscon phandle
> +          - description: MDIO clock enable register offset
> +          - description: MDIO reset request register offset
> +          - description: MDIO reset done register offset
> +          - description: MDIO reset deassert register offset

[Severity: Medium]
Do the last two argument cell descriptions match how the driver reads
them?

hns_mdio_probe() in drivers/net/ethernet/hisilicon/hns_mdio.c reads the
last two cells as status registers. It gets the disable and deassert
offsets by adding 4 to the first two cells:

drivers/net/ethernet/hisilicon/hns_mdio.c:hns_mdio_probe() {
    ...
	mdio_dev->sc_reg.mdio_clk_en =
		(u16)reg_args.args[0];
	mdio_dev->sc_reg.mdio_clk_dis =
		(u16)reg_args.args[0] + 4;
	mdio_dev->sc_reg.mdio_reset_req =
		(u16)reg_args.args[1];
	mdio_dev->sc_reg.mdio_reset_dreq =
		(u16)reg_args.args[1] + 4;
	mdio_dev->sc_reg.mdio_clk_st =
		(u16)reg_args.args[2];
	mdio_dev->sc_reg.mdio_reset_st =
		(u16)reg_args.args[3];
    ...
}

hns_mdio_reset() then polls mdio_clk_st and mdio_reset_st after it
writes the enable/disable and request/deassert registers.

The in-tree users agree with the driver. hip06.dtsi and hip07.dtsi have:

    subctrl-vbase = <&peri_c_subctrl 0x338 0xa38 0x531c 0x5a1c>;

These values are MDIO_SC_CLK_EN, MDIO_SC_RESET_REQ, MDIO_SC_CLK_ST and
MDIO_SC_RESET_ST. MDIO_SC_RESET_DREQ (0xa3c) is RESET_REQ + 4, not a
cell.

Suppose a new DT follows this schema and puts the reset deassert offset
in the last cell. Then hns_mdio_reset() would poll the wrong register and
time out, and the MDIO bus would fail to register.

Should the last two cells be described as the MDIO clock status and
MDIO reset status register offsets? It could also help to say that the
clock disable and reset deassert registers sit at +4 from the first two
offsets.

> +
> +required:
> +  - compatible
> +  - reg
> +  - '#address-cells'
> +  - '#size-cells'

[Severity: Low]
Should subctrl-vbase be added to the required list?

On DT systems the driver does not seem to work without it. In
hns_mdio_probe(), if of_parse_phandle_with_fixed_args() fails to find
subctrl-vbase, the driver only warns. It leaves subctrl_vbase as NULL
and still calls of_mdiobus_register(). The bus reset callback then
fails:

hns_mdio_probe()
  of_mdiobus_register()
    __mdiobus_register()
      bus->reset() == hns_mdio_reset()

drivers/net/ethernet/hisilicon/hns_mdio.c:hns_mdio_reset() {
    ...
	if (dev_of_node(bus->parent)) {
		if (!mdio_dev->subctrl_vbase) {
			dev_err(&bus->dev, "mdio sys ctl reg has not mapped\n");
			return -ENODEV;
		}
    ...
}

__mdiobus_register() then fails with that error.

So a node without subctrl-vbase passes dt_binding_check, but the bus
can never come up. The example in this schema is one such node.

Both in-tree users, hip06.dtsi and hip07.dtsi, already set the property.
Making it required would not break any existing DTs.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925215911.2950356-1-robh%40kernel.org