Re: [PATCH net-next v3 1/8] dt-bindings: net: add Xilinx TSN Endpoint Ethernet MAC

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In Documentation/devicetree/bindings/net/xlnx,tsn-endpoint-
  ethernet-mac.yaml, the ethernet@ node's dma-names (lines 81-96) does…
- [Low] Concern: tx-queues-config only enforces a lower bound tied to
  xlnx,num-priorities, and the rest of the checking is left to a driver…
- [Low] The child compatibles in
  Documentation/devicetree/bindings/net/xlnx,tsn-endpoint-ethernet-…

--

dt-bindings: net: add Xilinx TSN Endpoint Ethernet MAC

This adds a YAML binding for the Xilinx TSN Endpoint Ethernet MAC IP in
its Bridged Endpoint configuration. The wrapper node owns the shared
clocks and xlnx,num-priorities. It has two children: an xlnx,tsn-ep-mac
endpoint that holds the DMA channels, and an xlnx,tsn-switch three-port
switch with its MDIO buses and ports.

> 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 0000000000000..bca834120b3b1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/xlnx,tsn-endpoint-ethernet-mac.yaml
> @@ -0,0 +1,491 @@

[ ... ]

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

[ ... ]

> +patternProperties:
> +  "^ethernet@[0-9a-f]+$":
> +    type: object
> +    $ref: /schemas/net/ethernet-controller.yaml#
> +    unevaluatedProperties: false
> +    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

[Severity: Low]
The wrapper compatible is versioned (xlnx,tsn-endpoint-ethernet-mac-3.0).
The child compatibles here and on the switch node (xlnx,tsn-switch) are
generic and carry no version.

writing-bindings.rst asks for specific compatibles, with no device-family
names. For sub-blocks of a bigger device, it asks for device-based
compatibles.

If a later IP revision, or the planned Endpoint-only variant, changes the
endpoint or switch register layout, how would these child nodes be told
apart? The endpoint driver later in the series matches only on
"xlnx,tsn-ep-mac" in xlnx_tsn_ep_of_match.

Should the child compatibles be tied to the device or version, the same
way the wrapper compatible is?

[ ... ]

> +      dma-names:
> +        minItems: 2
> +        maxItems: 16
> +        items:
> +          pattern: "^(tx|rx)_chan[0-7]$"
> +        allOf:
> +          - contains:
> +              pattern: "^tx_chan[0-7]$"
> +          - contains:
> +              pattern: "^rx_chan[0-7]$"
> +        description: |
> +          Names matching the entries in 'dmas', one per DMA channel and in
> +          the same order. TX channel names use "tx_chan<N>" and RX channel
> +          names use "rx_chan<N>", where N is 0..7. At least one TX and one RX
> +          channel are required. Example: "tx_chan0", "tx_chan1", ...,
> +          "rx_chan0", "rx_chan1", ....

[Severity: Low]
The description says the names are "one per DMA channel". The v3
changelog says "Cap TX at 8 and RX at 8 channels". Does the schema
enforce either of these?

Without uniqueItems: true, dt_binding_check appears to accept a 16-entry
list of fifteen "tx_chan0" plus one "rx_chan0". It also appears to accept
"tx_chan0".."tx_chan7", then "tx_chan0".."tx_chan6", then "rx_chan0".

dtschema doesn't add uniqueness to -names properties by itself. For
example, renesas,drif.yaml lists "rx", "rx", while renesas,sdhi.yaml sets
uniqueItems: true on its clock-names.

Could uniqueItems: true be added here? The pattern only allows eight
names per direction, so that would also enforce the 8 TX / 8 RX limit.

Later in the series, ep_count_dma_channels() rejects duplicate TX and RX
names at probe time. So the runtime result is a DT that passes schema
validation and then fails probe.

[ ... ]

> +        patternProperties:
> +          "^queue[0-7]$":
> +            type: object
> +            additionalProperties: false

[ ... ]

> +        required:
> +          - queue0
> +          - queue1

[ ... ]

> +allOf:
> +  - if:
> +      properties:
> +        xlnx,num-priorities:
> +          const: 3
> +    then:
> +      patternProperties:
> +        "^ethernet@[0-9a-f]+$":
> +          properties:
> +            tx-queues-config:
> +              required:
> +                - queue0
> +                - queue1
> +                - queue2

[Severity: Low]
With xlnx,num-priorities = <2>, can a tx-queues-config that contains
queue0 through queue7 still pass dt_binding_check?

The ^queue[0-7]$ pattern accepts every index from 0 to 7. The if/then
blocks for num-priorities 3..8 only add entries to required. So the
schema has a lower bound tied to xlnx,num-priorities, but no upper bound.

Later in the series, ep_parse_tx_queue_config() rejects queueN with
N >= num_tx_queues at probe time, so only schema validation is affected.

Could each if/then block also set queueK: false for K >= N, so the
schema rejects these nodes too?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016-patches_v2_external-v3-0-e33d3960ca1e%40amd.com