Re: [PATCH net-next v4 2/6] dt-bindings: net: xlnx,xxv-ethernet: Add Xilinx XXV 10G/25G Ethernet

["Gupta, Suraj" <suraj.gupta2@amd.com>]

On 10/10/2026 5:25 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 1 · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] The XXV binding never says how link management works, so a node
>    that passes the schema, including both examples in the binding, cannot…
> - [Medium] The binding has no way to describe the XXV transceiver (GT)
>    reference clock.
> - [Low] The new XXV binding's `dma-names` schema only checks a per-item
>    pattern (`^[tr]x_chan([0-9]|1[0-5])$`) and a count of 2 to 32.
> 
> --
> 
> dt-bindings: net: xlnx,xxv-ethernet: Add Xilinx XXV 10G/25G Ethernet
> 
> This adds a new DT binding schema for the Xilinx XXV 10G/25G Ethernet IP.
> The IP has an in-core PCS/PMA and uses an external AXI DMA or MCDMA. The
> binding covers the 10gbase-r/25gbase-r phy-modes, the rx_core/drp/s_axi
> clocks and the dmas/dma-names properties.
> 
>> diff --git a/Documentation/devicetree/bindings/net/xlnx,xxv-ethernet.yaml b/Documentation/devicetree/bindings/net/xlnx,xxv-ethernet.yaml
>> new file mode 100644
>> index 0000000000000..cf31ab677a4bc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/xlnx,xxv-ethernet.yaml
>> @@ -0,0 +1,110 @@
> 
> [ ... ]
> 
>> +description:
>> +  The Xilinx XXV Ethernet Subsystem implements a 10G/25G Ethernet MAC with an
>> +  in-core PCS/PMA driving a high-speed serial transceiver. It supports the
>> +  10GBASE-R and 25GBASE-R line rates, with 25G operation following the 25G
>> +  Ethernet Consortium specification. Because the PCS/PMA is integrated into
>> +  the IP, the subsystem has no MDIO bus master and is not managed through an
>> +  external MDIO PHY; link state is read from the MAC's own status registers.
> 
> [Severity: High]
> The binding says there is no external MDIO PHY. It does not require or
> show managed = "in-band-status" or a fixed-link node, though. Neither
> example has phy-handle, fixed-link or managed.
> 
> The XXV support added later in this series sets only mac_capabilities and
> supported_interfaces in axienet_xxv_phylink_set_capabilities(). It does
> not set phylink_config.default_an_inband, so cfg_link_an_mode stays at
> MLO_AN_PHY.
> 
> On open this gives:
> 
> axienet_open()
>    phylink_fwnode_phy_connect(lp->phylink, dev_fwnode(lp->dev), 0)
>      fwnode_get_phy_node() fails, since there is no phy-handle
>      if (pl->cfg_link_an_mode == MLO_AN_PHY)
>          return -ENODEV;
> 
> A node that matches this binding, including either example, would then
> fail ndo_open with "phylink_of_phy_connect() failed". Is that expected?
> 
> Also, phylink_pcs_poll_start() only polls the internal PCS when
> cfg_link_an_mode == MLO_AN_INBAND.
> 
> Should the binding require managed: const: in-band-status? The other
> option is for the XXV phylink_set_caps to set default_an_inband = true.

Correct. The XXV subsystem has an integrated PCS/PMA and no external 
PHY, so link is managed in-band from the MAC status registers. The 
driver should therefore default to in-band mode.

> Either way, could the binding document the chosen mode?
> 
>> +  Any SFP module management (e.g. I2C) is handled by separate IP outside this
>> +  binding. Its payload is moved by an external AXI DMA (or multichannel DMA)
>> +  controller referenced through the "dmas" property.
> 
> [ ... ]
> 
>> +  clocks:
>> +    items:
>> +      - description: RX AXI4-Stream clock.
>> +      - description: GT DRP clock.
>> +      - description: AXI4-Lite register clock.
>> +
>> +  clock-names:
>> +    items:
>> +      - const: rx_core
>> +      - const: drp
>> +      - const: s_axi
> 
> [Severity: Medium]
> How would a DT describe the transceiver (GT) reference clock here? The
> description says the in-core PCS/PMA drives a high-speed serial
> transceiver, and that transceiver has its own reference clock input.
> 
> The clocks/clock-names list is fixed at three entries, and
> unevaluatedProperties is false. A DT that also lists the GT refclk would
> fail validation. xlnx,axi-ethernet.yaml already documents mgt_clk for its
> optional internal PCS/PMA PHY.
> 
> Later in the series, axienet_10g25g_clk_init() claims only s_axi, rx_core
> and drp. axienet_xxv_poll_link_ready() then fails open when GT reset-done
> times out:
> 
>      netdev_err(ndev, "XXV MAC GT reset not complete! Cross-check the MAC ref clock configuration\n");
> 
> On boards where a Linux-managed, gateable clock provider supplies the GT
> refclk, nothing claims that clock. Unused clock cleanup could then gate
> it. This depends on the board.
> 
> Could an optional trailing clock (minItems: 3) be added now?
> writing-bindings.rst asks for complete bindings even when the driver does
> not use every feature.
> 
Agreed. The three current clocks (rx_core -> rx_core_clk, drp -> dclk, 
s_axi -> s_axi_aclk) are correct, but per PG210 the GT has a dedicated 
differential reference clock (refclk_p0/refclk_n0 -> tx_serdes_refclk) 
that the list cannot express today.
I'll add an optional trailing clock, mirroring mgt_clk in 
xlnx,axi-ethernet.yaml

> [ ... ]
> 
>> +  dma-names:
>> +    items:
>> +      pattern: "^[tr]x_chan([0-9]|1[0-5])$"
>> +    description:
>> +      Should be "tx_chan0", "tx_chan1" ... "tx_chan15" for DMA Tx channel and
>> +      "rx_chan0", "rx_chan1" ... "rx_chan15" for DMA Rx channel.
>> +    minItems: 2
>> +    maxItems: 32
> 
> [Severity: Low]
> This checks the name pattern and the count. It does not require
> "tx_chan0" and "rx_chan0", or even one TX and one RX entry. Both of these
> pass dt_binding_check:
> 
>      dma-names = "tx_chan1", "rx_chan1";
>      dma-names = "tx_chan0", "tx_chan1";
> 
> The driver requests fixed names, though:
> 
> axienet_probe()
>      tx_chan = dma_request_chan(lp->dev, "tx_chan0");
>      if (IS_ERR(tx_chan))
>          return dev_err_probe(lp->dev, PTR_ERR(tx_chan), "No Ethernet DMA (TX) channel found\n");
> 
> axienet_init_dmaengine()
>      lp->tx_chan = dma_request_chan(lp->dev, "tx_chan0");
>      ...
>      lp->rx_chan = dma_request_chan(lp->dev, "rx_chan0");
> 
> Since dmas is required, XXV always takes this path. Could contains
> constraints for "tx_chan0" and "rx_chan0" be added? Another option is to
> fix the leading entries with items/additionalItems.
> 
> The pattern came from xlnx,axi-ethernet.yaml, but this patch creates a
> new binding with it.
> 

Agreed, the driver hard-requests "tx_chan0"/"rx_chan0". For v5 I'll add 
contains constraints so at least one tx_chan0 and one rx_chan0 are 
required, while still allowing the additional MCDMA channels.

Thanks,
Suraj

> [ ... ]
>