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

quoted

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

quoted

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

[ ... ]

quoted

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