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

Wei Fang
2026-03-23T10:33:09Z

> On Mon, Mar 23, 2026 at 02:07:49PM +0800, Wei Fang wrote:
> > +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_REVMII |
> 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;
>
> I would prefer newer drivers not to use phylink_autoneg_inband()
> anymore. Note that there is no need to support RGMII inband in the
> kernel (nor is there any proper support without a "phylink_pcs"
> being present to provide the inband status.)

Thanks for the info, I will remove it.

>
> > +static void netc_port_set_hd_flow_control(struct netc_port *np, bool en)
> > +{
> > + if (!np->caps.half_duplex)
> > + return;
> > +
> > + /* The HD_FCEN is used in conjunction with the PM_HD_FLOW_CTRL
> > + * register, which has a default value, so currently we do not
> > + * set it in the driver. The half duplex flow control works by
> > + * the backpressure, and the backpressure is essentially just
> > + * a long preamble transmitted on the link intended to create
> > + * a collision and get the half duplex link partner to defer.
> > + */
> > + netc_mac_port_rmw(np, NETC_PM_CMD_CFG(0),
> PM_CMD_CFG_HD_FCEN,
> > + en ? PM_CMD_CFG_HD_FCEN : 0);
>
> We don't support half duplex backpressure in the kernel. I notice
> you always enable this whenever HD mode is negotiated, which means
> there's no way for the user to disable it. Flow control can cause
> problems. Ethernet relies on packet dropping for congestion
> management.

Okay, make sense, I will remove it.

>
> > +static void imx94_switch_phylink_get_caps(int port,
> > + struct phylink_config *config)
> > +{
> > + config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> > + MAC_1000FD;
> > +
> > + switch (port) {
> > + case 0 ... 1:
> > + __set_bit(PHY_INTERFACE_MODE_SGMII,
> > + config->supported_interfaces);
> > + __set_bit(PHY_INTERFACE_MODE_1000BASEX,
> > + config->supported_interfaces);
> > + __set_bit(PHY_INTERFACE_MODE_2500BASEX,
> > + config->supported_interfaces);
> > + config->mac_capabilities |= MAC_2500FD;
> > + fallthrough;
> > + case 2:
> > + config->mac_capabilities |= MAC_10 | MAC_100;
> > + __set_bit(PHY_INTERFACE_MODE_MII,
> > + config->supported_interfaces);
> > + __set_bit(PHY_INTERFACE_MODE_RMII,
> > + config->supported_interfaces);
> > + if (port == 2)
> > + __set_bit(PHY_INTERFACE_MODE_REVMII,
> > + config->supported_interfaces);
>
> The "case 2" above already ensures that port is 2 here.

"case 0...1" uses "fallthrough" instead of "break", and port 0 and 1 do not
support REVMII. So the check is needed.