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

Wei Fang
2026-03-19T02:02:07Z

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