Re: [PATCH 01/20] dt-bindings: net: add Xilinx TSN Endpoint Ethernet MAC

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
dt-bindings: net: add Xilinx TSN Endpoint Ethernet MAC

This adds a new binding for the Xilinx TSN Endpoint Ethernet MAC IP,
describing a wrapper node that owns the six IP clocks and
xlnx,num-priorities, plus an endpoint MAC child and a three-port switch
child.  A MAINTAINERS entry is added for the new file.

A few questions about the schema below.

> diff --git a/Documentation/devicetree/bindings/net/xlnx,tsn-endpoint-ethernet-mac.yaml b/Documentation/devicetree/bindings/net/xlnx,tsn-endpoint-ethernet-mac.yaml
> new file mode 100644
> index 000000000000..00f52b7b623c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/xlnx,tsn-endpoint-ethernet-mac.yaml
> @@ -0,0 +1,343 @@

[ ... ]

> +properties:
> +  compatible:
> +    const: xlnx,tsn-endpoint-ethernet-mac-3.0
> +
> +  reg:
> +    maxItems: 1

[Severity: Low]
Should the wrapper node have a reg at all?  It is listed in the top-level
required list alongside ranges, and in the example the two describe the
same window:

    reg = <0x80040000 0x40000>;
    ranges = <0x0 0x80040000 0x40000>;

Every child window (mac1 0x0+0x14000, ep-mac 0x16000+0xa000, mac2
0x20000+0x14000, switch 0x38000+0x8000) falls inside the parent's own
reg.

The commit message says the wrapper only owns the six shared clocks and
xlnx,num-priorities, and the wrapper driver in this series never maps
that region:

drivers/net/ethernet/xilinx/tsn/xilinx_tsn_main.c:tsn_ip_probe() {
    ...
    ret = devm_clk_bulk_get(dev, TSN_NUM_CLOCKS, w->clks);
    ...
    return devm_of_platform_populate(dev);
}

Would it be cleaner for a bus node that translates its children through
ranges to either drop reg or describe only a wrapper-private register
block that no child window overlaps?

> +
> +  clocks:
> +    items:
> +      - description: GTX clock for transmitter logic and RTC
> +      - description: GTX 90-degree phase shifted clock for RGMII
> +      - description: Host RX FIFO clock
> +      - description: Host TX FIFO clock
> +      - description: Reference clock for IDELAY control
> +      - description: AXI-Lite register interface clock
> +
> +  clock-names:
> +    items:
> +      - const: gtx
> +      - const: gtx90
> +      - const: host_rxfifo
> +      - const: host_txfifo
> +      - const: ref
> +      - const: s_axi

[Severity: Low]
Are all six clocks present in every IP configuration?

A fixed items list implies minItems == maxItems == 6, and both clocks and
clock-names are in the top-level required list, so each DT must supply all
six phandles.  Two of them are documented as RGMII specific, gtx90 ("GTX
90-degree phase shifted clock for RGMII") and ref ("Reference clock for
IDELAY control"), while the switch ports below also allow:

              phy-mode:
                enum:
                  - gmii
                  - rgmii
                  - rgmii-id

For a GMII-only build with no IDELAY primitives, would a DT author have to
invent phandles for clocks the IP does not instantiate?  If those two are
optional in some configurations, could they be made conditional rather
than unconditionally required?

Note the wrapper driver in this series is consistent with what is written
here, it requests exactly these six names non-optionally via
devm_clk_bulk_get(dev, TSN_NUM_CLOCKS, w->clks), so the question is only
about the hardware.

[ ... ]

> +patternProperties:
> +  "^ep-mac@":
> +    type: object
> +    $ref: /schemas/net/ethernet-controller.yaml#
> +    description:
> +      Endpoint MAC sub-node. Provides the host-side data path through the
> +      DMA channels described by its dmas / dma-names.
> +    properties:
> +      compatible:
> +        const: xlnx,tsn-ep-mac
> +
> +      reg:
> +        maxItems: 1

[ ... ]

> +    required:
> +      - compatible
> +      - reg
> +      - dmas
> +      - dma-names
> +      - tx-queues-config
> +
> +    unevaluatedProperties: false

[Severity: Low]
How is an Endpoint-only instance described here?  The top-level
description says the IP:

  "provides low-latency network connectivity in either Endpoint-only or
   Bridged Endpoint configurations"

and

  "It also supports GMII/RGMII interfaces for connection to an external
   PHY"

The ep-mac node declares no interrupts/interrupt-names, and
ethernet-controller.yaml contains no interrupt properties either, so with
unevaluatedProperties: false any interrupt or MDIO child on the endpoint
MAC is rejected.  All interrupts and both MDIO buses live only on the
optional switch@ child, whose interrupt-names list is fixed to MAC1/MAC2
plus one ptp_timer.

An Endpoint-only DT (wrapper plus ep-mac, no switch child) is allowed by
the top-level required list, but ends up with no interrupt and no MDIO
description at all, and there is one compatible string with no if/then to
tell the two configurations apart.

Should ep-mac gain its own interrupts/interrupt-names and a way to reach
its PHY, or should the description be narrowed to the Bridged Endpoint
configuration the drivers in this series implement?  Nothing appears
broken today, the endpoint driver requests no interrupts and drives the
data path entirely through dmaengine.

> +
> +  "^switch@":
> +    type: object
> +    $ref: /schemas/net/dsa/dsa.yaml#/$defs/ethernet-ports

[ ... ]

> +      mdio-mac1:
> +        $ref: /schemas/net/mdio.yaml#
> +        description:
> +          MDIO bus controller for MAC1. Registers reside inside the mac1
> +          reg-names window.
> +        unevaluatedProperties: false
> +
> +      mdio-mac2:
> +        $ref: /schemas/net/mdio.yaml#
> +        description:
> +          MDIO bus controller for MAC2. Registers reside inside the mac2
> +          reg-names window.
> +        unevaluatedProperties: false

[Severity: Low]
This isn't a bug, but the child node names contradict the $nodename
patterns of the generic schemas this binding references.

mdio.yaml documents:

    pattern: '^mdio(-(bus|external))?(@.+|-([0-9]+))?$'

and -mac1 / -mac2 matches none of the allowed suffixes (-bus, -external,
@<addr>, -<digits>).  Similarly the patternProperties key "^ep-mac@"
forces names like the example's ep-mac@16000, while
ethernet-controller.yaml documents:

    pattern: "^ethernet(@.*)?$"

Other in-tree bindings that $ref mdio.yaml for a child use a conforming
name (qca8k.yaml, realtek.yaml, renesas,rzn1-a5psw.yaml,
renesas,etheravb.yaml, faraday,ftgmac100.yaml, microchip,lan7800.yaml).
Would mdio@<offset> (or mdio-0 / mdio-1) and ethernet@<offset> work here
instead?

Related: the mdio-mac<N> nodes carry no reg, so nothing in DT locates the
MDIO block inside the parent mac1/mac2 window.  The offset is instead
hard-coded in the driver added later in this series:

drivers/net/dsa/xilinx/xilinx_tsn.h:
    #define TSN_MDIO_MC_OFFSET		0x00000500

Could the buses be described as mdio@500 with a reg so the offset comes
from DT?

> +
> +      ethernet-ports:
> +        type: object
> +        additionalProperties: true
> +
> +        properties:
> +          port@0:
> +            type: object
> +            additionalProperties: true
> +            description:
> +              CPU port. Wired internally to the ep-mac sibling.
> +
> +            properties:
> +              reg:
> +                const: 0
> +
> +              phy-mode:
> +                const: internal
> +
> +        patternProperties:
> +          "^port@[1-2]$":
> +            type: object
> +            additionalProperties: true
> +            description:
> +              External Ethernet port (port@1 is wired to MAC1, port@2 to
> +              MAC2). Its PHY is reached through the corresponding
> +              mdio-mac<N> bus.
> +
> +            properties:
> +              phy-mode:
> +                enum:
> +                  - gmii
> +                  - rgmii
> +                  - rgmii-id

[Severity: Medium]
Can a valid device tree bypass all of these port constraints?

The switch node uses $ref: /schemas/net/dsa/dsa.yaml#/$defs/ethernet-ports,
and the referenced schemas accept both container and port spellings:

Documentation/devicetree/bindings/net/ethernet-switch.yaml:
    patternProperties:
      '^(ethernet-)?ports$':
    ...
        patternProperties:
          '^(ethernet-)?port@[0-9a-f]+$':
    ...
    oneOf:
      - required:
          - ports
      - required:
          - ethernet-ports

The local constraints are attached only under properties: ethernet-ports:
and only to the keys port@0 and "^port@[1-2]$", and that container is
declared additionalProperties: true.

So a DT spelling the container ports { port@0 ... } (the spelling most
existing DSA device trees use), or spelling the ports
ethernet-port@0/1/2, passes dt_binding_check while none of the hardware
rules apply: the CPU port's reg: const: 0 and phy-mode: const: internal,
the gmii/rgmii/rgmii-id enum on external ports, and the port@1/port@2
only topology of this three-port IP.  A fourth port or phy-mode =
"sgmii" then validates cleanly and is only caught at probe:

drivers/net/dsa/xilinx/xilinx_tsn.c:xlnx_tsn_setup() {
    ...
    if (!dsa_is_user_port(ds, XLNX_TSN_PORT_MAC1) ||
        !dsa_is_user_port(ds, XLNX_TSN_PORT_MAC2))
	    return dev_err_probe(sw->dev, -EINVAL,
			         "both MAC1 and MAC2 must be enabled as switch ports\n");
    ...
}

Would matching both container spellings ('^(ethernet-)?ports$') and both
port spellings, and dropping additionalProperties: true inside the
container, close this?

[ ... ]