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

Hi Rob,

On 8/14/2026 11:34 PM, Rob Herring wrote:

On Fri, Aug 14, 2026 at 03:59:27PM +0530, Neeli, Srinivas wrote:

quoted

Hi Jakub,

On 8/9/2026 1:18 AM, Jakub Kicinski wrote:

quoted

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.

quoted

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 @@

[ ... ]

quoted

+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?

Thanks. We would prefer to keep reg as a required property of the wrapper,
for two reasons.

First, reg describes the whole TSN IP register window, which is a hardware
property of the IP, and ranges translates the child offsets within it. The
child windows do not fully cover the IP window, mac1, ep-mac, mac2 and the
switch fabric account for 232 KB of the 256 KB window, leaving 24 KB
unmapped by any child (0x14000..0x16000 and 0x34000..0x38000). That reserved
space is where the IP places top-level registers that are not owned by any
child node.

Why not describe those 2 ranges in reg? Otherwise, you are mapping the
child addresses twice potentially.

quoted

Second, we intend to use that space. A planned use is a wrapper-level
build/configuration register that tells software which sub-blocks the IP
instantiates, so the wrapper can decide which children to populate. For
example, an endpoint-only build has no switch block, and the wrapper would
read that register to skip the switch node instead of relying on the child
nodes alone. That register lives in the wrapper window, not in any child, so
the wrapper needs its own reg to reach it.

Because reg is in the required list, making it optional later and then
requiring it again would be an ABI change. Keeping it required now avoids
that
churn once the wrapper starts reading its own register. The wrapper driver
in
this series does not map reg yet, which is a driver limitation rather than a
reason to drop the hardware description.

If there are registers outside of the child nodes, then absolutely you
should have 'reg'. But please describe what registers and functionality
are there.

Sorry, I had misunderstood the IP here. My thinking was that we might
eventually want a common register in the wrapper to drive some behavior,
and that reserving the space for it now would be cleaner.

I checked with the IP team and that is not the case. There is no such
register in the wrapper, and the reserved memory is only there for future
extension of the child nodes (EP/switch/MAC), not for the wrapper.

So I will drop the reg property from the wrapper node and keep ranges for
the child translation.

[...]

quoted

quoted

quoted

+      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?

  Thanks, accepted. In next version renames ep-mac@ to ethernet@ and
mdio-mac1/mdio-mac2 to
   mdio-1/mdio-2 to match the referenced schemas. The MDIO registers sit at a
   fixed offset inside the per-MAC window the driver already maps and have no
   standalone address, so the mdio-1/2 nodes keep that form without a reg
   rather than mdio@500.

If there is a register address associated with them, then using that is
strongly preferred over mdio-N.

Rob

The MDIO block sits at a fixed offset (0x500), but it isn't a separately
decoded window. It's a few registers inside the MAC core register space,
which the binding already describes as one window via reg-names
"mac1"/"mac2". Giving it its own mdio@500 with a reg would overlap that MAC
window, so I will follow your previous advice and use mdio-1 / mdio-2 form that
mdio.yaml allows.

Thanks
Srinivas Neeli