Re: [PATCH v8 net-next 11/15] net: dsa: netc: add phylink MAC operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `netc_mac_port_rmw()` function can silently fail to synchronize the pMAC and eMAC registers due to a flawed early return optimization.
- [High] The `netc_port_mac_tx_graceful_stop()` function fails to wait for the switch port to become idle (`PSR_TX_BUSY`), risking trapped data and FIFO overflows.
- [High] Missing MAC-level speed, duplex, and PCS configuration for SGMII and 2500BASE-X interfaces renders them non-functional.
--

commit 1c39a10b5175a16ff2753fc6b076dd4c1bbc15de
Author: Wei Fang <wei.fang@nxp.com>
net: dsa: netc: add phylink MAC operations

This commit adds .phylink_get_caps() to struct netc_switch_info to handle
different numbers of ports and MAC capabilities across NETC switch versions.
It implements the phylink_mac_ops callbacks .mac_config(), .mac_link_up(),
and .mac_link_down().

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> index 8e3a3230226c3..2141b3aa96b7a 100644
> --- a/drivers/net/dsa/netc/netc_main.c
> +++ b/drivers/net/dsa/netc/netc_main.c

[ ... ]

> +static void netc_mac_port_rmw(struct netc_port *np, u32 reg,
> +			      u32 mask, u32 val)
> +{
> +	u32 old, new;
> +
> +	if (is_netc_pseudo_port(np))
> +		return;
> +
> +	WARN_ON((mask | val) != mask);
> +
> +	old = netc_port_rd(np, reg);
> +	new = (old & ~mask) | val;
> +	if (new == old)
> +		return;

Does this early return prevent the pMAC register from synchronizing correctly
if the bootloader or previous state left the eMAC and pMAC desynchronized?

Since old is read exclusively from the eMAC, if the new value matches the
eMAC's current value but the pMAC's state differs, returning early skips writing
to the pMAC register entirely. This could leave the preemptable MAC operating
with incorrect settings.

> +
> +	netc_port_wr(np, reg, new);
> +	if (np->caps.pmac)
> +		netc_port_wr(np, reg + NETC_PMAC_OFFSET, new);
> +}

[ ... ]

> +static void netc_port_mac_tx_graceful_stop(struct netc_port *np)
> +{
> +	netc_port_rmw(np, NETC_POR, POR_TXDIS, POR_TXDIS);
> +
> +	if (is_netc_pseudo_port(np))
> +		return;
> +
> +	netc_port_wait_tx_empty(np, 0);

Could this disable the MAC TX prematurely, trapping data in the pipeline?

The code disables the Switch Port from feeding new data and then waits for the
MAC TX FIFO to empty. However, it doesn't wait for the Switch Port to finish
pushing its current data into the MAC (PSR_TX_BUSY == 1). 

If the MAC momentarily drains its FIFO to the wire faster than the port fills
it, PM_IEVENT_TX_EMPTY might briefly read true while the port is still
actively transmitting data into the MAC.

Should we poll !(val & PSR_TX_BUSY) from NETC_PSR immediately after setting
POR_TXDIS, similar to how the RX path waits for PSR_RX_BUSY before disabling
the port?

> +	if (np->caps.pmac)
> +		netc_port_wait_tx_empty(np, 1);
> +
> +	netc_mac_port_rmw(np, NETC_PM_CMD_CFG(0), PM_CMD_CFG_TX_EN, 0);
> +}

[ ... ]

> +static void netc_mac_link_up(struct phylink_config *config,
> +			     struct phy_device *phy, unsigned int mode,
> +			     phy_interface_t interface, int speed,
> +			     int duplex, bool tx_pause, bool rx_pause)
> +{
> +	struct dsa_port *dp = dsa_phylink_to_port(config);
> +	struct netc_port *np;
> +
> +	np = NETC_PORT(dp->ds, dp->index);
> +	netc_port_set_speed(np, speed);
> +
> +	if (phy_interface_mode_is_rgmii(interface))
> +		netc_port_set_rgmii_mac(np, speed, duplex);
> +
> +	if (interface == PHY_INTERFACE_MODE_RMII ||
> +	    interface == PHY_INTERFACE_MODE_REVMII ||
> +	    interface == PHY_INTERFACE_MODE_MII)
> +		netc_port_set_rmii_mii_mac(np, speed, duplex);

Are SGMII and 2500BASE-X interfaces left unconfigured at the MAC level?

The driver advertises support for PHY_INTERFACE_MODE_SGMII and
PHY_INTERFACE_MODE_2500BASEX in imx94_switch_phylink_get_caps(), including
10/100 half-duplex capabilities for SGMII.

However, MAC-level speed and duplex configurations are bypassed here for SGMII
and 2500BASE-X, and the driver doesn't implement the .mac_select_pcs callback
nor configure in-band autonegotiation. Without these, the interfaces might fail
to establish a correct link, especially at lower speeds or half-duplex.

> +
> +	netc_port_mac_tx_enable(np);
> +	netc_port_mac_rx_enable(np);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518082506.1318236-1-wei.fang@nxp.com?part=11