Re: [PATCH v12 4/4] net: dsa: add basic initial driver for MxL862xx switches

On Sun, Feb 01, 2026 at 02:25:13AM +0000, Daniel Golle wrote:

Add very basic DSA driver for MaxLinear's MxL862xx switches.

In contrast to previous MaxLinear switches the MxL862xx has a built-in
processor that runs a sophisticated firmware based on Zephyr RTOS.
Interaction between the host and the switch hence is organized using a
software API of that firmware rather than accessing hardware registers
directly.

Add descriptions of the most basic firmware API calls to access the
built-in MDIO bus hosting the 2.5GE PHYs, basic port control as well as
setting up the CPU port.

Implement a very basic DSA driver using that API which is sufficient to
get packets flowing between the user ports and the CPU port.

The firmware offers all features one would expect from a modern switch
hardware, they are going to be added one by one in follow-up patch
series.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v12:
 * list switch variants in Kconfig starting with dash ('-')
 * remove the __packed attribute on structs which are
   naturally packed according to C rules (ie. one other struct
   with only a single byte-aligned member)
 * log error in mxl862xx_port_disable()
 * introduce !NULL check for return value of dsa_to_port() in
   mxl862xx_add_single_port_bridge
 * check cpu_dp being non-NULL before dereferencing dp->cpu_dp
 * use non-racy and deterministic name for MII bus
 * skip ports without cpu_dp assigned in mxl862xx_setup_cpu_bridge()
   to avoid potential NULL-pointer dereference
 * call dev_set_drvdata() only after dsa_register_switch() has been
   successfully completed

I have nothing else to comment except to request you to undo some of the
changes requested by AI review. Sorry I didn't have time to comment on v11.

+static int mxl862xx_add_single_port_bridge(struct dsa_switch *ds, int port)
+{
+	struct mxl862xx_bridge_port_config br_port_cfg = {};
+	struct dsa_port *dp = dsa_to_port(ds, port);
+	struct mxl862xx_bridge_alloc br_alloc = {};
+	int ret;
+
+	if (!dp)
+		return -ENODEV;

I'm sorry, I can't accept code that fears its own shadow. Some people,
when they see a defensive NULL check think there is a valid reason
behind it and will try to see what that reason is.

If dsa_to_port() would have possibly returned NULL, mxl862xx_port_setup() ->
dsa_is_cpu_port(), which _is_ implemented using dsa_to_port(), would
have already crashed the kernel and this test here would have been too
little, too late.

But the kernel doesn't crash there, because calling dsa_to_port() is
safe for these arguments, and this test is simply logically inconsistent
with previous unconditional in this call path.

+
+	ret = MXL862XX_API_READ(ds->priv, MXL862XX_BRIDGE_ALLOC, br_alloc);
+	if (ret) {
+		dev_err(ds->dev, "failed to allocate a bridge for port %d\n", port);
+		return ret;
+	}
+
+	br_port_cfg.bridge_id = br_alloc.bridge_id;
+	br_port_cfg.bridge_port_id = cpu_to_le16(port);
+	br_port_cfg.mask = cpu_to_le32(MXL862XX_BRIDGE_PORT_CONFIG_MASK_BRIDGE_ID |
+				       MXL862XX_BRIDGE_PORT_CONFIG_MASK_BRIDGE_PORT_MAP |
+				       MXL862XX_BRIDGE_PORT_CONFIG_MASK_MC_SRC_MAC_LEARNING |
+				       MXL862XX_BRIDGE_PORT_CONFIG_MASK_VLAN_BASED_MAC_LEARNING);
+	br_port_cfg.src_mac_learning_disable = true;
+	br_port_cfg.vlan_src_mac_vid_enable = false;
+	br_port_cfg.vlan_dst_mac_vid_enable = false;
+	if (dp->cpu_dp)
+		br_port_cfg.bridge_port_map[0] = cpu_to_le16(BIT(dp->cpu_dp->index));
+
+	return MXL862XX_API_WRITE(ds->priv, MXL862XX_BRIDGEPORT_CONFIGSET, br_port_cfg);
+}
+
+static int mxl862xx_setup_mdio(struct dsa_switch *ds)
+{
+	struct mxl862xx_priv *priv = ds->priv;
+	struct device *dev = ds->dev;
+	struct device_node *mdio_np;
+	struct mii_bus *bus;
+	int ret;
+
+	bus = devm_mdiobus_alloc(dev);
+	if (!bus)
+		return -ENOMEM;
+
+	bus->priv = priv;
+	ds->user_mii_bus = bus;
+	bus->name = KBUILD_MODNAME "-mii";
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(dev));
+	bus->read_c45 = mxl862xx_phy_read_c45_mii_bus;
+	bus->write_c45 = mxl862xx_phy_write_c45_mii_bus;
+	bus->read = mxl862xx_phy_read_mii_bus;
+	bus->write = mxl862xx_phy_write_mii_bus;
+	bus->parent = dev;
+	bus->phy_mask = ~ds->phys_mii_mask;
+
+	mdio_np = of_get_child_by_name(dev->of_node, "mdio");
+	if (!mdio_np)
+		return -ENODEV;

As per the dt-bindings patch, the "mdio" child node is not required.
But here it is.

+
+	ret = devm_of_mdiobus_register(dev, bus, mdio_np);
+	of_node_put(mdio_np);
+
+	return ret;
+}
+
+static int mxl862xx_wait_ready(struct dsa_switch *ds)
+{
+	struct mxl862xx_sys_fw_image_version ver = {};
+	unsigned long start = jiffies, timeout;
+	struct mxl862xx_priv *priv = ds->priv;
+	struct mxl862xx_cfg cfg = {};
+	int ret;
+
+	timeout = start + msecs_to_jiffies(MXL862XX_READY_TIMEOUT_MS);
+	msleep(2000); /* it always takes at least 2 seconds */
+	do {
+		ret = MXL862XX_API_READ_QUIET(priv, SYS_MISC_FW_VERSION, ver);
+		if (ret || !ver.iv_major)
+			goto not_ready_yet;
+
+		/* being able to perform CFGGET indicates that
+		 * the firmware is ready
+		 */
+		ret = MXL862XX_API_READ_QUIET(priv,
+					      MXL862XX_COMMON_CFGGET,
+					      cfg);
+		if (ret)
+			goto not_ready_yet;
+
+		dev_info(ds->dev, "switch ready after %ums, firmware %u.%u.%u (build %u)\n",
+			 jiffies_to_msecs(jiffies - start),
+			 ver.iv_major, ver.iv_minor,
+			 le16_to_cpu(ver.iv_revision),
+			 le32_to_cpu(ver.iv_build_num));
+		return 0;
+
+not_ready_yet:
+		msleep(MXL862XX_READY_POLL_MS);
+	} while (time_before(jiffies, timeout));
+
+	dev_err(ds->dev, "switch not responding after reset\n");
+	return -ETIMEDOUT;
+}
+
+static int mxl862xx_setup_cpu_bridge(struct dsa_switch *ds, int port)
+{
+	struct mxl862xx_bridge_port_config br_port_cfg = {};
+	struct mxl862xx_priv *priv = ds->priv;
+	u16 bridge_port_map = 0;
+	struct dsa_port *dp;
+
+	/* CPU port bridge setup */
+	br_port_cfg.mask = cpu_to_le32(MXL862XX_BRIDGE_PORT_CONFIG_MASK_BRIDGE_PORT_MAP |
+				       MXL862XX_BRIDGE_PORT_CONFIG_MASK_MC_SRC_MAC_LEARNING |
+				       MXL862XX_BRIDGE_PORT_CONFIG_MASK_VLAN_BASED_MAC_LEARNING);
+
+	br_port_cfg.bridge_port_id = cpu_to_le16(port);
+	br_port_cfg.src_mac_learning_disable = false;
+	br_port_cfg.vlan_src_mac_vid_enable = true;
+	br_port_cfg.vlan_dst_mac_vid_enable = true;
+
+	/* include all assigned user ports in the CPU portmap */
+	dsa_switch_for_each_user_port(dp, ds) {
+		if (!dp->cpu_dp)
+			continue;

All user ports are given a valid non-NULL dp->cpu_dp pointer. I strongly
oppose introducing FUD in the code. If there are valid reasons behind
this I'm all ears, but there aren't.

+
+		if (dp->cpu_dp->index != port)
+			continue;
+
+		bridge_port_map |= BIT(dp->index);
+	}
+	br_port_cfg.bridge_port_map[0] |= cpu_to_le16(bridge_port_map);
+
+	return MXL862XX_API_WRITE(priv, MXL862XX_BRIDGEPORT_CONFIGSET, br_port_cfg);
+}
+
+static int mxl862xx_setup(struct dsa_switch *ds)
+{
+	struct mxl862xx_priv *priv = ds->priv;
+	int ret;
+
+	ret = mxl862xx_reset(priv);
+	if (ret)
+		return ret;
+
+	ret = mxl862xx_wait_ready(ds);
+	if (ret)
+		return ret;
+
+	return mxl862xx_setup_mdio(ds);
+}
+
+static int mxl862xx_port_setup(struct dsa_switch *ds, int port)
+{
+	bool is_cpu_port = dsa_is_cpu_port(ds, port);
+	int ret;
+
+	/* disable port and flush MAC entries */
+	ret = mxl862xx_port_state(ds, port, false);
+	if (ret)
+		return ret;
+
+	mxl862xx_port_fast_age(ds, port);
+
+	/* skip setup for unused and DSA ports */
+	if (dsa_is_unused_port(ds, port) ||
+	    dsa_is_dsa_port(ds, port))
+		return 0;

Each of dsa_is_cpu_port(), dsa_is_unused_port(), dsa_is_dsa_port() are
implemented using dsa_to_port(), and that loops over dst->ports in order
to find "dp" just to return dp->index.

OTOH if you call dsa_to_port() only once, we have dsa_port_is_cpu(),
dsa_port_is_unused(), dsa_port_is_dsa() which can be called on that and
have lower complexity. Please do that.

You do _not_ need to test whether dsa_to_port() returns NULL, if "ds"
and "port" were passed to you by DSA in the context of a DSA callback.
Any exception to this rule will be explicitly noted in the documentation.

+
+	/* configure tag protocol */
+	ret = mxl862xx_configure_sp_tag_proto(ds, port, is_cpu_port);
+	if (ret)
+		return ret;
+
+	/* assign CTP port IDs */
+	ret = mxl862xx_configure_ctp_port(ds, port, port,
+					  is_cpu_port ? 32 - port : 1);
+	if (ret)
+		return ret;
+
+	if (is_cpu_port)
+		/* assign user ports to CPU port bridge */
+		return mxl862xx_setup_cpu_bridge(ds, port);
+
+	/* setup single-port bridge for user ports */
+	return mxl862xx_add_single_port_bridge(ds, port);
+}
+
+static void mxl862xx_phylink_get_caps(struct dsa_switch *ds, int port,
+				      struct phylink_config *config)
+{
+	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | MAC_10 |
+				   MAC_100 | MAC_1000 | MAC_2500FD;
+
+	__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+		  config->supported_interfaces);
+}
+
+static const struct dsa_switch_ops mxl862xx_switch_ops = {
+	.get_tag_protocol = mxl862xx_get_tag_protocol,
+	.setup = mxl862xx_setup,
+	.port_setup = mxl862xx_port_setup,
+	.phylink_get_caps = mxl862xx_phylink_get_caps,
+	.port_enable = mxl862xx_port_enable,
+	.port_disable = mxl862xx_port_disable,
+	.port_fast_age = mxl862xx_port_fast_age,
+};
+
+static void mxl862xx_phylink_mac_config(struct phylink_config *config,
+					unsigned int mode,
+					const struct phylink_link_state *state)
+{
+}
+
+static void mxl862xx_phylink_mac_link_down(struct phylink_config *config,
+					   unsigned int mode,
+					   phy_interface_t interface)
+{
+}
+
+static void mxl862xx_phylink_mac_link_up(struct phylink_config *config,
+					 struct phy_device *phydev,
+					 unsigned int mode,
+					 phy_interface_t interface,
+					 int speed, int duplex,
+					 bool tx_pause, bool rx_pause)
+{
+}
+
+static const struct phylink_mac_ops mxl862xx_phylink_mac_ops = {
+	.mac_config = mxl862xx_phylink_mac_config,
+	.mac_link_down = mxl862xx_phylink_mac_link_down,
+	.mac_link_up = mxl862xx_phylink_mac_link_up,
+};
+
+static int mxl862xx_probe(struct mdio_device *mdiodev)
+{
+	struct device *dev = &mdiodev->dev;
+	struct mxl862xx_priv *priv;
+	struct dsa_switch *ds;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->mdiodev = mdiodev;
+
+	ds = devm_kzalloc(dev, sizeof(*ds), GFP_KERNEL);
+	if (!ds)
+		return -ENOMEM;
+
+	priv->ds = ds;
+	ds->dev = dev;
+	ds->priv = priv;
+	ds->ops = &mxl862xx_switch_ops;
+	ds->phylink_mac_ops = &mxl862xx_phylink_mac_ops;
+	ds->num_ports = MXL862XX_MAX_PORTS;
+
+	ret = dsa_register_switch(ds);
+	if (ret)
+		return ret;
+
+	dev_set_drvdata(dev, ds);

There is no point in doing this, calling dev_set_drvdata() before and
then simply "return dsa_register_switch()" is perfectly fine. You also
won't need the "ret" variable.

+
+	return 0;
+}
+
+static void mxl862xx_remove(struct mdio_device *mdiodev)
+{
+	struct dsa_switch *ds = dev_get_drvdata(&mdiodev->dev);
+
+	if (!ds)
+		return;
+
+	dsa_unregister_switch(ds);
+}
+
+static void mxl862xx_shutdown(struct mdio_device *mdiodev)
+{
+	struct dsa_switch *ds = dev_get_drvdata(&mdiodev->dev);
+
+	if (!ds)
+		return;
+
+	dsa_switch_shutdown(ds);
+
+	dev_set_drvdata(&mdiodev->dev, NULL);
+}
+
+static const struct of_device_id mxl862xx_of_match[] = {
+	{ .compatible = "maxlinear,mxl86282" },
+	{ .compatible = "maxlinear,mxl86252" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mxl862xx_of_match);
+
+static struct mdio_driver mxl862xx_driver = {
+	.probe  = mxl862xx_probe,
+	.remove = mxl862xx_remove,
+	.shutdown = mxl862xx_shutdown,
+	.mdiodrv.driver = {
+		.name = "mxl862xx",
+		.of_match_table = mxl862xx_of_match,
+	},
+};
+
+mdio_module_driver(mxl862xx_driver);
+
+MODULE_DESCRIPTION("Driver for MaxLinear MxL862xx switch family");
+MODULE_LICENSE("GPL");