Re: [PATCH 13/20] net: dsa: xilinx: wire up phylink for the switch ports

[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.
---
net: dsa: xilinx: wire up phylink for the switch ports

This wires up the MAC-side phylink ops for the three switch ports:
phylink_get_caps advertises RGMII variants, SGMII and GMII plus MAC_100FD
and MAC_1000FD on the two external user ports and PHY_INTERFACE_MODE_INTERNAL
with MAC_1000FD on the CPU port, while mac_link_up/mac_link_down program the
speed-config field and the TX_EN / RX_EN bits.

> diff --git a/drivers/net/dsa/xilinx/xilinx_tsn.c b/drivers/net/dsa/xilinx/xilinx_tsn.c
> index 542b74514ed3..86aba2e8b6e9 100644
> --- a/drivers/net/dsa/xilinx/xilinx_tsn.c
> +++ b/drivers/net/dsa/xilinx/xilinx_tsn.c

[ ... ]

> @@ -324,6 +325,105 @@ static void xlnx_tsn_port_stp_state_set(struct dsa_switch *ds, int port,
>  	xlnx_tsn_set_port_state(sw, port, hw_state);
>  }
>  
> +static void xlnx_tsn_phylink_get_caps(struct dsa_switch *ds, int port,
> +				      struct phylink_config *config)
> +{
> +	if (port == XLNX_TSN_CPU_PORT) {
> +		config->mac_capabilities = MAC_1000FD;
> +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +			  config->supported_interfaces);
> +		return;
> +	}
> +
> +	/* The MAC's speed-config field only encodes 100 / 1000.
> +	 * Half-duplex and 10 Mbps are not supported.
> +	 */
> +	config->mac_capabilities = MAC_100FD | MAC_1000FD;
> +	phy_interface_set_rgmii(config->supported_interfaces);
> +	__set_bit(PHY_INTERFACE_MODE_SGMII, config->supported_interfaces);
> +	__set_bit(PHY_INTERFACE_MODE_GMII, config->supported_interfaces);
> +}

[Severity: Low]
Should the set of interface modes advertised here match the DT binding
added earlier in this series?

xlnx_tsn_phylink_get_caps() advertises six modes for ports 1 and 2:
phy_interface_set_rgmii() sets PHY_INTERFACE_MODE_RGMII, RGMII_ID,
RGMII_RXID and RGMII_TXID, and SGMII and GMII are set explicitly.

Documentation/devicetree/bindings/net/xlnx,tsn-endpoint-ethernet-mac.yaml
allows a narrower set for the same ports:

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

and the top-level description repeats the narrower claim:

  It also supports GMII/RGMII interfaces for connection
  to an external PHY, enabling full-duplex operation at 100 Mb/s and
  1 Gb/s speeds.

If the MACs really can do SGMII and the rgmii-rxid / rgmii-txid delay
variants, does a board synthesised that way fail dtbs_check because the
schema rejects the phy-mode value the driver supports?

If instead the binding is the accurate description, does this advertise
interface modes the hardware does not implement?

> +static void xlnx_tsn_mac_config(struct phylink_config *config,
> +				unsigned int mode,
> +				const struct phylink_link_state *state)
> +{
> +	/* Interface mode (RGMII / SGMII / GMII) is fixed at IP synthesis
> +	 * time. There is no runtime register to program it here.
> +	 */
> +}

Since mac_config is empty and has no error path, phylink would resolve the
link for any of the advertised modes, so a mismatch would show up as a
silently dead port rather than a probe or configuration failure. Would it
make sense to either extend the binding enum or drop the extra
supported_interfaces bits so the two agree?

Both files are still in this state at the end of the series, so this does
not appear to be resolved by a later patch.