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

Simon Horman
2026-03-18T14:47:15Z

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: dsa: netc: add phylink MAC operations

This commit adds phylink MAC operations to the NETC DSA switch driver,
including support for different PHY interface modes (RGMII, RMII, MII,
REVMII, SGMII, 2500BASE-X).

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> index bc7d48b99610..884ee899fc89 100644
> --- a/drivers/net/dsa/netc/netc_main.c
> +++ b/drivers/net/dsa/netc/netc_main.c

[ ... ]

> @@ -595,10 +595,221 @@ static void netc_switch_get_ip_revision(struct netc_switch *priv)
> priv->revision = val & IPBRR0_IP_REV;
> }
>
> +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;

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

> + int speed, int duplex)
> +{
> + u32 mask, val = 0;
> +
> + mask = PM_IF_MODE_ENA | PM_IF_MODE_SSP | PM_IF_MODE_HD |
> + PM_IF_MODE_M10;
> +
> + if (speed == SPEED_10)
> + val |= PM_IF_MODE_M10;
> +
> + if (duplex != DUPLEX_FULL)
> + val |= PM_IF_MODE_HD;
> +
> + netc_mac_port_rmw(np, NETC_PM_IF_MODE(0), mask, val);
> +}

[ ... ]

> +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) &&
> + !phylink_autoneg_inband(mode)) {
> + netc_port_force_set_rgmii_mac(np, speed, duplex);
> + }
> +
> + if (interface == PHY_INTERFACE_MODE_RMII ||
> + interface == PHY_INTERFACE_MODE_REVMII ||
> + interface == PHY_INTERFACE_MODE_MII) {
> + net_port_set_rmii_mii_mac(np, speed, duplex);
> + }
> +
> + netc_port_set_hd_flow_control(np, duplex == DUPLEX_HALF);
> + netc_port_mac_rx_enable(np);
> +}
> +
> +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));
> +}
> +
> +static const struct phylink_mac_ops netc_phylink_mac_ops = {
> + .mac_config = netc_mac_config,
> + .mac_link_up = netc_mac_link_up,
> + .mac_link_down = netc_mac_link_down,
> +};
> +
> static const struct dsa_switch_ops netc_switch_ops = {
> .get_tag_protocol = netc_get_tag_protocol,
> .setup = netc_setup,
> .teardown = netc_teardown,
> + .phylink_get_caps = netc_phylink_get_caps,
> };
>
> static int netc_switch_probe(struct pci_dev *pdev,
> @@ -639,6 +850,7 @@ static int netc_switch_probe(struct pci_dev *pdev,
> ds->num_ports = priv->info->num_ports;
> ds->num_tx_queues = NETC_TC_NUM;
> ds->ops = &netc_switch_ops;
> + ds->phylink_mac_ops = &netc_phylink_mac_ops;
> ds->priv = priv;
>
> priv->ds = ds;

[ ... ]