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

[Jakub Kicinski]

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: netc: add phylink MAC operations

Different versions of NETC switches have different numbers of ports and
MAC capabilities, so add .phylink_get_caps() to struct netc_switch_info.
In addition, related interfaces of struct phylink_mac_ops are added.

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> index 5828fd3e342e..f11f5d0f6a6d 100644
> --- a/drivers/net/dsa/netc/netc_main.c
> +++ b/drivers/net/dsa/netc/netc_main.c
[ ... ]
> +static void netc_port_set_mac_mode(struct netc_port *np,
> +				   unsigned int mode,
> +				   phy_interface_t phy_mode)
> +{
> +	u32 mask = PM_IF_MODE_IFMODE | PM_IF_MODE_REVMII;
> +	u32 val = 0;
> +
> +	switch (phy_mode) {
> +	case PHY_INTERFACE_MODE_RGMII:
[ ... ]
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		val |= IFMODE_SGMII;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	netc_mac_port_rmw(np, NETC_PM_IF_MODE(0), mask, val);
> +}

In imx94_switch_phylink_get_caps(), PHY_INTERFACE_MODE_1000BASEX is
advertised as supported for ports 0 and 1. Does it need to be handled
in this switch statement? As written, it appears it will fall through
to the default case and clear the IFMODE bits, leaving the MAC in an
undefined mode.

[ ... ]
> +static void netc_port_mac_rx_enable(struct netc_port *np)
> +{
> +	netc_port_rmw(np, NETC_POR, PCR_RXDIS, 0);
> +	netc_mac_port_rmw(np, NETC_PM_CMD_CFG(0), PM_CMD_CFG_RX_EN,
> +			  PM_CMD_CFG_RX_EN);
> +}

Pseudo ports (like the CPU port) bypass PMAC register accesses during
graceful stop via is_netc_pseudo_port(). Should there be a similar check
here, and in netc_port_set_mac_mode(), to prevent accessing physical MAC
registers on pseudo ports?

Additionally, when np->caps.pmac is true, netc_port_mac_rx_graceful_stop()
disables the preemption MAC via NETC_PM_CMD_CFG(1). Should this function
re-enable it when bringing the link back up?

> +static void netc_port_wait_rx_empty(struct netc_port *np, int mac)
> +{
> +	u32 val;
> +
> +	if (read_poll_timeout(netc_port_rd, val, val & PM_IEVENT_RX_EMPTY,
> +			      100, 10000, false, np, NETC_PM_IEVENT(mac)))
> +		dev_warn(np->switch_priv->dev,
> +			 "MAC %d of swp%d RX is not empty\n", mac,
> +			 np->dp->index);
> +}

Is NETC_PM_IEVENT a sticky write-1-to-clear register? If the RX FIFO had
previously emptied during normal operation, could this bit already be set,
causing read_poll_timeout() to return immediately without waiting?

> +static void netc_port_mac_rx_graceful_stop(struct netc_port *np)
> +{
> +	u32 val;
> +
> +	if (is_netc_pseudo_port(np))
> +		goto check_rx_busy;
> +
> +	if (np->caps.pmac) {
> +		netc_port_rmw(np, NETC_PM_CMD_CFG(1), PM_CMD_CFG_RX_EN, 0);
> +		netc_port_wait_rx_empty(np, 1);
> +	}
> +
> +	netc_port_rmw(np, NETC_PM_CMD_CFG(0), PM_CMD_CFG_RX_EN, 0);
> +	netc_port_wait_rx_empty(np, 0);
> +
> +check_rx_busy:
> +	if (read_poll_timeout(netc_port_rd, val, !(val & PSR_RX_BUSY),
> +			      100, 10000, false, np, NETC_PSR))
> +		dev_warn(np->switch_priv->dev, "swp%d RX is busy\n",
> +			 np->dp->index);
> +
> +	netc_port_rmw(np, NETC_POR, PCR_RXDIS, PCR_RXDIS);
> +}

For pseudo ports, the code jumps directly to check_rx_busy and polls for
PSR_RX_BUSY to clear. Since PCR_RXDIS is only set after the polling
completes, will the RX pipeline continue accepting traffic and cause this
poll to always timeout on an active link?

> +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);
> +
> +	netc_port_mac_rx_enable(np);
> +}

The callback imx94_switch_phylink_get_caps() advertises MAC_ASYM_PAUSE and
MAC_SYM_PAUSE capabilities. Is it expected that tx_pause and rx_pause are
ignored here? If flow control is implemented in a subsequent patch, should
the advertisement be deferred until then?

> +static void netc_mac_link_down(struct phylink_config *config,
> +			       unsigned int mode,
> +			       phy_interface_t interface)
> +{
> +	struct dsa_port *dp = dsa_phylink_to_port(config);
> +
> +	netc_port_mac_rx_graceful_stop(NETC_PORT(dp->ds, dp->index));
> +}

This correctly stops the RX pipeline, but should the TX path
(PM_CMD_CFG_TX_EN or PCR_TXDIS) also be disabled when the link goes down?