Re: [net-next,11/14] net: dsa: netc: add phylink MAC operations
From: Simon Horman <horms@kernel.org>
Date: 2026-03-18 14:47:11
Also in:
imx, linux-arm-kernel, linux-devicetree, lkml, netdev
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).
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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);
+}[ ... ]
quoted hunk ↗ jump to hunk
+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;
[ ... ]