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