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

[Wei Fang]

> > +static void netc_phylink_get_caps(struct dsa_switch *ds, int port,
> > +				  struct phylink_config *config)
> > +{
> > +	struct netc_switch *priv = ds->priv;
> > +
> > +	priv->info->phylink_get_caps(port, config);
> > +}
> > +
> > +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_ENA;
> > +	u32 val = 0;
> > +
> > +	switch (phy_mode) {
> > +	case PHY_INTERFACE_MODE_RGMII:
> > +	case PHY_INTERFACE_MODE_RGMII_ID:
> > +	case PHY_INTERFACE_MODE_RGMII_RXID:
> > +	case PHY_INTERFACE_MODE_RGMII_TXID:
> > +		val |= IFMODE_RGMII;
> > +		/* Enable auto-negotiation for the MAC if its
> > +		 * RGMII interface supports In-Band status.
> > +		 */
> > +		if (phylink_autoneg_inband(mode))
> > +			val |= PM_IF_MODE_ENA;
> > +		break;
> > +	case PHY_INTERFACE_MODE_RMII:
> > +		val |= IFMODE_RMII;
> > +		break;
> > +	case PHY_INTERFACE_MODE_REVMII:
> > +		val |= PM_IF_MODE_REVMII;
> > +		fallthrough;
> > +	case PHY_INTERFACE_MODE_MII:
> > +		val |= IFMODE_MII;
> > +		break;
> 
> Does the mask need to include PM_IF_MODE_REVMII? Looking at
> netc_port_set_mac_mode(), the mask is set to:
> 
>     u32 mask = PM_IF_MODE_IFMODE | PM_IF_MODE_ENA;
> 
> which expands to GENMASK(2,0) | BIT(15) = 0x8007. When the REVMII
> case sets val |= PM_IF_MODE_REVMII (BIT(3)), this makes val = 0x9.
> 
> In netc_mac_port_rmw(), there's a check:
> 
>     WARN_ON((mask | val) != mask)
> 
> This check would fire on every REVMII configuration since 0x800F != 0x8007.
> 
> Additionally, REVMII is reachable because imx94_switch_phylink_get_caps()
> advertises PHY_INTERFACE_MODE_REVMII as supported for port 2. When
> transitioning FROM REVMII to another mode, PM_IF_MODE_REVMII (BIT(3))
> won't be cleared since it's not in the mask, leaving stale configuration.
> 
> Should the mask be:
> 
>     u32 mask = PM_IF_MODE_IFMODE | PM_IF_MODE_REVMII |
> PM_IF_MODE_ENA;

Thanks for pointing this issue, I will fix it.

> 
> > +	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);
> > +}
> 
> [ ... ]
> 
> > +static void netc_port_force_set_rgmii_mac(struct netc_port *np,
> > +					  int speed, int duplex)
> > +{
> > +	u32 mask, val;
> > +
> > +	mask = PM_IF_MODE_ENA | PM_IF_MODE_SSP | PM_IF_MODE_HD |
> > +	       PM_IF_MODE_M10 | PM_IF_MODE_REVMII;
> > +
> > +	switch (speed) {
> > +	default:
> > +	case SPEED_1000:
> > +		val = FIELD_PREP(PM_IF_MODE_SSP, SSP_1G);
> > +		break;
> > +	case SPEED_100:
> > +		val = FIELD_PREP(PM_IF_MODE_SSP, SSP_100M);
> > +		break;
> > +	case SPEED_10:
> > +		val = FIELD_PREP(PM_IF_MODE_SSP, SSP_10M);
> > +		break;
> > +	}
> > +
> > +	if (duplex != DUPLEX_FULL)
> > +		val |= PM_IF_MODE_HD;
> > +
> > +	netc_mac_port_rmw(np, NETC_PM_IF_MODE(0), mask, val);
> > +}
> > +
> > +static void net_port_set_rmii_mii_mac(struct netc_port *np,
> 
> This isn't a bug, but should this function be named
> netc_port_set_rmii_mii_mac() instead of net_port_set_rmii_mii_mac()?
> All other functions in this driver use the netc_ prefix
> (netc_port_set_speed, netc_port_force_set_rgmii_mac,
> netc_port_set_mac_mode, etc.).

Oh, sorry for this typo, I will correct the name.