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

[Neeli, Srinivas]

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:
> > Hi Jakub,
> >
> > On 8/9/2026 1:18 AM, Jakub Kicinski wrote:
> > > 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?
> > 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.
>
> > 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.

> [...]
>
> > > > +      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