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

[Wei Fang]

> > +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?

netc_mac_port_rmw() has checked whether the port is pseudo MAC. See
below.

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;

	[...]
}
> 
> 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?

netc_mac_port_rmw() also set preemption MAC if the port supports preemtion.

static void netc_mac_port_rmw(struct netc_port *np, u32 reg,
			      u32 mask, u32 val)
{
	[...]
	if (np->caps.pmac)
		netc_port_wr(np, reg + NETC_PMAC_OFFSET, new);
}

> 
> > +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?

NETC_PM_IEVENT[RX_EMPTY] is a read only bit. It will be cleared
if Rx FIFO not empty or Rx packet receive in process. And it will be
set if Rx FIFO is empty and no Rx packet receive in process.

> 
> > +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?

For the pseudo ports, no need to check the PSR_RX_BUSY bit, only need
to set PCR_RXDIS bit to discard RX packets. I will improve it.

> 
> > +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?

Okay, I will move the advertisement to the subsequent patch.