Re: [PATCH net-next v3 1/4] dt-bindings: net: dsa: add MT7628 ESW

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email says "Since Andrew requested some changes already here is the
AI review to look thru as well (could be false positives)."
---
Subject: dt-bindings: net: dsa: add MT7628 ESW

This patch adds a new YAML device tree binding for the Embedded Switch
block found in the MediaTek MT7628 SoC. The binding describes a five
port switch with integrated 10/100 PHYs plus an internally connected
1G CPU port, together with reset lines and an optional MDIO subnode.

> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7628-esw.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7628-esw.yaml
> new file mode 100644
> index 000000000000..a9db9057ee54
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7628-esw.yaml
> @@ -0,0 +1,101 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/dsa/mediatek,mt7628-esw.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek MT7628 Embedded Ethernet Switch
> +
> +maintainers:
> +  - Joris Vaisvila <joey@tinyisr.com>

This isn't a bug, but the maintainers list only contains the submitter
at an unrelated domain. Existing in-tree MediaTek DSA bindings such as
Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml list
multiple MediaTek or long-standing MediaTek-community maintainers.

For an official MediaTek SoC's built-in switch IP, would it make sense
to CC or co-list a MediaTek networking maintainer (Sean Wang, Landen
Chao, Daniel Golle, ...) so the hardware description can be
corroborated and the binding ABI has long-term stewardship?

> +
> +description:
> +  The MT7628 SoC's built-in Ethernet Switch is a five port switch with
> +  integrated 10/100 PHYs. The switch registers are directly mapped in the SoC's
> +  memory. The switch has an internally connected 1G CPU port and 5 user ports
> +  connected to the built-in Fast Ethernet PHYs.

Is the description internally consistent?

The first sentence says "a five port switch with integrated 10/100
PHYs", but the following sentences describe "an internally connected
1G CPU port and 5 user ports", which is six ports in total. The
example below also enumerates six ports (reg = 0..4 and 6).

Could this be reworded as e.g. "5 user ports plus 1 internal CPU
port" to avoid the ambiguity?

Separately, there is no justification in the commit message or
binding for the absence of a clocks property. MT7628 peripherals are
typically gated by syscon clock outputs (see
arch/mips/boot/dts/ralink/mt7628a.dtsi), and
drivers/net/ethernet/mediatek/mtk_eth_soc.c already treats "esw" as a
named clock.

The MT7628 ESW driver added alongside this binding deliberately does
not call any clk_* API, which is a valid design choice if the esw
gate is always on after reset. Since DT bindings are ABI, though,
adding a clocks property later would be a backward-incompatible
change.

Could the commit message explain why no clocks property is needed, or
alternatively could clocks/clock-names be described in the binding
now even if the initial driver ignores them?

> +
> +unevaluatedProperties: false
> +
> +allOf:
> +  - $ref: dsa.yaml#/$defs/ethernet-ports
> +
> +properties:
> +  compatible:
> +    const: mediatek,mt7628-esw
> +
> +  reg:
> +    maxItems: 1
> +
> +  resets:
> +    items:
> +      - description: internal switch block reset
> +      - description: internal phy package reset
> +
> +  reset-names:
> +    items:
> +      - const: esw
> +      - const: ephy
> +
> +  mdio:
> +    $ref: /schemas/net/mdio.yaml#
> +    unevaluatedProperties: false

The mdio subnode is referenced here and is actually consumed by the
driver (drivers/net/dsa/mt7628.c mt7628_setup_internal_mdio() calls
of_get_available_child_by_name(..., "mdio") and
devm_of_mdiobus_register()), but the binding provides no description
of when or why a user would populate it.

Given the top-level description says the PHYs are "integrated" and
"internally connected", when should an mdio subnode be present? Is an
external MDIO master ever supported?

A short description attached to the mdio property would make the
intent clear to DT authors.

> +
> +required:
> +  - compatible
> +  - reg
> +  - resets
> +  - reset-names
> +  - ethernet-ports

Should the valid port reg values be constrained here?

The MT7628 ESW has a fixed, well-known set of valid port indices
(0..4 for user ports and 6 for the CPU, with port 5 not existing).
See mt7628_phylink_get_caps() in drivers/net/dsa/mt7628.c, which
explicitly comments that "port 5 does not exist on MT7628".

As written the schema accepts ethernet-port@5 or ethernet-port@7 at
dt_binding_check time, even though the driver will silently ignore
them. Would adding a reg enum constraint on the ethernet-port
children (e.g. via patternProperties, or an override of the
$defs/ethernet-ports reg) catch these mistakes at schema-validation
time?

> +
> +examples:
> +  - |
> +    switch0: switch@10110000 {
> +        compatible = "mediatek,mt7628-esw";
> +
> +        reg = <0x10110000 0x8000>;
> +
> +        resets = <&sysc 23>, <&sysc 24>;
> +        reset-names = "esw", "ephy";
> +
> +        ethernet-ports {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
[ ... ]
> +            ethernet-port@6 {
> +                reg = <6>;
> +                phy-mode = "internal";
> +                ethernet = <&ethernet>;

The example references an undefined phandle here: ethernet-port@6
uses ethernet = <&ethernet> but no node labelled "ethernet" is defined
in the example snippet.

Other DSA binding examples in Documentation/devicetree/bindings/net/dsa/
typically either provide a minimal stub for referenced phandles inside
the example or use a reg-based reference. Will dt_binding_check emit a
warning about the unresolved phandle as written?

> +
> +                fixed-link {
> +                    speed = <1000>;
> +                    full-duplex;
> +                };
> +            };
> +        };
> +    };
-- 
pw-bot: cr