Message-ID: <aXo9MnBFuaj8d5Hv@makrotopia.org>
Date: Wed, 28 Jan 2026 16:45:38 +0000
From: Daniel Golle <daniel@...rotopia.org>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	Simon Horman <horms@...nel.org>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	Frank Wunderlich <frankwu@....de>, Chad Monroe <chad@...roe.io>,
	Cezary Wilmanski <cezary.wilmanski@...ran.com>,
	Avinash Jayaraman <ajayaraman@...linear.com>,
	Bing tao Xu <bxu@...linear.com>, Liang Xu <lxu@...linear.com>,
	Juraj Povazanec <jpovazanec@...linear.com>,
	"Fanni (Fang-Yi) Chan" <fchan@...linear.com>,
	"Benny (Ying-Tsan) Weng" <yweng@...linear.com>,
	"Livia M. Rosu" <lrosu@...linear.com>,
	John Crispin <john@...ozen.org>
Subject: Re: [PATCH net-next v9 4/4] net: dsa: add basic initial driver for
 MxL862xx switches

Hi Vladimir,

thank you for the elaborate and very helpful review (one could even say
"inspiring" ;).

See some questions and answers inline below.

On Wed, Jan 28, 2026 at 01:29:50PM +0200, Vladimir Oltean wrote:
> > [...]
> > +/**
> > + * struct mxl862xx_bridge_port_alloc - Bridge Port Allocation
> > + * @bridge_port_id: If the bridge port allocation is successful, a valid ID
> > + *                  will be returned in this field. Otherwise, INVALID_HANDLE is returned.
> > + *                  For bridge port free, this field should contain a valid ID returned by
> > + *                  the bridge port allocation. ID 0 is special for the CPU port in
> > + *                  PRX300, mapping to CTP (Connectivity Termination Port) 0 (Logical Port
> > + *                  0 with Sub-interface ID 0), and is pre-allocated during initialization.
> 
> Just to be clear, "CPU port in PRX300" does not refer to the "DSA CPU
> port" that connects to the Linux system, no? Because
> mxl862xx_configure_tag_proto() assigns a different CTP ID for the DSA
> CPU port AFAICS, not 0.
> 
> Without further clarifications, this comment is a bit confusing. I would
> at least add that CTP 0 is not used by the driver.

This documentation was probably meant for a whole series of different
chips which are used with the (proprietary) switch driver which in case
of the MxL862xx is running on the internal microcontroller rather than
on the host OS...

I'll remove the comment, PRX300 is a xPON SoC which contains the same
switch IP. There are plans to upstream support also for this SoC, but of
course without the proprietary switch driver which this API refers to.

> [...]
> > +/**
> > + * struct mxl862xx_ctp_port_assignment - CTP Port Assignment/association
> > + *                                       with logical port
> > + * @logical_port_id: Logical Port Id. The valid range is hardware dependent
> > + * @first_ctp_port_id: First CTP (Connectivity Termination Port) ID mapped
> > + *                     to above logical port ID
> > + * @number_of_ctp_port: Total number of CTP Ports mapped above logical port
> > + *                      ID
> > + * @mode: Logical port mode to define sub interface ID format. See
> > + *        &enum mxl862xx_logical_port_mode
> > + * @bridge_port_id: Bridge Port ID (not FID). For allocation, each CTP
> > + *                  allocated is mapped to the Bridge Port given by this
> > + *                  field. The Bridge Port will be configured to use first CTP
> > + *                  as egress CTP.
> > + *
> > + * A CTP (Connectivity Termination Port) is equivalent to
> > + * Logical Port + Sub-Interface-ID (capped at 31).
> 
> "+" as in "arithmetical plus"?

Yes.

> How do we know that CTP 5 is 3 + 2 and not 4 + 1?

We can't. This "rule" anyway seems to rather be just a software
convention, see below.

> > [...]
> > +static int mxl862xx_configure_tag_proto(struct dsa_switch *ds, int port, bool enable)
> > +{
> > +	struct mxl862xx_ctp_port_assignment assign = {
> > +		.number_of_ctp_port = cpu_to_le16(enable ? (32 - DSA_MXL_PORT(port)) : 1),
> > +		.logical_port_id = DSA_MXL_PORT(port),
> > +		.first_ctp_port_id = cpu_to_le16(DSA_MXL_PORT(port)),
> > +		.mode = cpu_to_le32(MXL862XX_LOGICAL_PORT_ETHERNET),
> 
> I have a really hard time understanding the physical port to CTP port
> mapping here. Specifically, I see you allocate 1 CTP port (sub-interface)
> per user port, and the rest up to 32 CTP ports as sub-interfaces of the
> DSA CPU port.
> 
> But I can only see this working if the index of the DSA CPU port is the
> last. Otherwise, won't the CTP port IDs of user ports subsequent to the
> DSA CPU port overlap with sub-interfaces of the latter?
> 
> Example:
> mxl862xx_configure_tag_proto(ds, 3, true) // CPU port
> -> .number_of_ctp_port = cpu_to_le16(32 - DSA_MXL_PORT(3)), // 28
> -> .logical_port_id = DSA_MXL_PORT(3), // 4
> -> .first_ctp_port_id = cpu_to_le16(DSA_MXL_PORT(3)), // 4
> mxl862xx_configure_tag_proto(ds, 4, false) // user port
> -> .number_of_ctp_port = cpu_to_le16(1), // 1
> -> .logical_port_id = DSA_MXL_PORT(4), // 5
> -> .first_ctp_port_id = cpu_to_le16(DSA_MXL_PORT(4)), // 5
> 
> Doesn't the CTP port ID of user port 4 (5) collide with sub-interface 2
> of CPU port 3?

Absolutely true. It works coincidental because only SerDes port 0
(port_id 9) is allowed to be CPU port. When using SerDes port 1
(port_id 13) as a user port it already starts to be tricky and more care
needs to be taken when setting up CTP port assignment...

Now that I understand more of what it actually does I'll try to come up
with something smarter.

> > [...]
> > +static int mxl862xx_isolate_port(struct dsa_switch *ds, int port)
> 
> I would recommend against using the "isolate" term in this context, it
> will conflict at some point with the meaning of the BR_ISOLATED bridge
> port flag. From man bridge:
> 
>        isolated on or isolated off
>               Controls whether a given port will be isolated, which
>               means it will be able to communicate with non-isolated
>               ports only.  By default this flag is off.
> 
> Something like gswip_add_single_port_br() is better maybe?

+1

> > [...]
> > +static int mxl862xx_setup(struct dsa_switch *ds)
> > +{
> > +	struct mxl862xx_bridge_port_config br_port_cfg = {};
> > +	struct mxl862xx_priv *priv = ds->priv;
> > +	u16 bridge_port_map = 0;
> > +	struct dsa_port *dp;
> > +	int cpu_port = -1;
> > +	int ret;
> > +
> > +	dsa_switch_for_each_cpu_port(dp, ds) {
> > +		/* Only a single CPU port is supported by now */
> > +		if (cpu_port != -1)
> > +			return -EINVAL;
> 
> If the hardware supports multiple CPU ports, then please describe all
> CPU ports as such in the device tree, and make an effort to handle that
> description gracefully even if you cannot make use of the second CPU port.

The number of available CTP (32) limits the possible setup for CPU ports.
The reference driver only allows port_id 9 (SerDes 0) to be used as CPU
port. SerDes 1 may also be used as CPU port in theory.

> See the rules by which dsa_tree_setup_cpu_ports() creates the initial
> user to CPU port mapping. This avoids a future breakage you'll cause
> with old kernels when you update the device tree to describe the second
> CPU port for what it is.

Understood. On old kernel, lets assume with this basic driver just added
but future DT with more than one CPU port described one should still end
up with the first CPU port used and all ports assigned to that CPU port.
Right?

> 
> > +
> > +		cpu_port = dp->index;
> > +	}
> > +
> > +	ret = mxl862xx_reset(priv);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = mxl862xx_wait_ready(ds);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* 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(DSA_MXL_PORT(cpu_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 non-CPU ports in the CPU portmap */
> > +	dsa_switch_for_each_available_port(dp, ds) {
> > +		if (dsa_port_is_cpu(dp))
> > +			continue;
> > +
> > +		bridge_port_map |= BIT(DSA_MXL_PORT(dp->index));
> > +	}
> > +	br_port_cfg.bridge_port_map[0] |= cpu_to_le16(bridge_port_map);
> > +
> > +	ret = MXL862XX_API_WRITE(priv, MXL862XX_BRIDGEPORT_CONFIGSET,
> > +				 br_port_cfg);
> > +	if (ret) {
> > +		dev_err(ds->dev, "failed to set the CPU portmap\n");
> > +		return ret;
> > +	}
> > +	mxl862xx_port_fast_age(ds, cpu_port);
> 
> Duplicate with the call from mxl862xx_port_setup()?

+1 I'll remove that.

> > [...]
> > +static int mxl862xx_port_setup(struct dsa_switch *ds, int port)
> > +{
> > +	bool is_cpu_port = dsa_is_cpu_port(ds, port);
> > +	int ret;
> > +
> > +	ret = mxl862xx_configure_tag_proto(ds, port, is_cpu_port);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!is_cpu_port && !dsa_is_unused_port(ds, port)) {
> > +		ret = mxl862xx_isolate_port(ds, port);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	mxl862xx_port_fast_age(ds, port);
> > +
> > +	ret = mxl862xx_port_state(ds, port, is_cpu_port);
> 
> What is the point of this? Do CPU ports need to be enabled prior to the
> mxl862xx_port_enable() call?

Testing turned out it doesn't matter and it's totally fine to have
the CPU port enabled/disabled by calling mxl862xx_port_enable() just
like for any other port.

> 
> > [...]
> > +#define MXL862XX_MAX_PHY_PORT_NUM	8
> > +#define MXL862XX_MAX_EXT_PORT_NUM	7
> > +#define MXL862XX_MAX_PORT_NUM		(MXL862XX_MAX_PHY_PORT_NUM + \
> > +					 MXL862XX_MAX_EXT_PORT_NUM)
> 
> 8 + 7 == 15. This is written into ds->num_ports.

Good you are asking this. It's another oddity which came from the vendor
driver. It's wrong in many ways, see below.

> 
> Could you please explain how this agrees with the Kconfig help text:
> 	  These switches have two 10GE SerDes interfaces, one typically
> 	  used as CPU port.
> 	   MxL86282 has eight 2.5 Gigabit PHYs
> 	   MxL86252 has five 2.5 Gigabit PHYs
> 
> What are the extra port indices used for, and is it ok that MxL86282 and
> MxL86252 are registered with the same ds->num_ports value? This should
> be visible in the "devlink port" command - even unused ports have
> devlink instances.

This is probably because the SerDes ports also support 10G QXGMII PHYs
with 4x 2.5G TP ports connected via a single pair of 10G SerDes lanes.
In this case 4 port indexes are used for the 4 ports.

My understanding of the internal port IDs by now:
	 MxL86282				MxL86252
port 0:  microcontroller			microcontroller
port 1:  PHY port 0				PHY port 0
port 2:  PHY port 1				PHY port 1
port 3:  PHY port 2				PHY port 2
port 4:  PHY port 3				PHY port 3
port 5:  PHY port 4				PHY port 4
port 6:  PHY port 5				n/c
port 7:  PHY port 6				n/c
port 8:  PHY port 7				n/c
port 9:  SerDes PCS 0				SerDes PCS 0
port 10: SerDes PCS 0 (QXGMII)			SerDes PCS 0 (QXGMII)
port 11: SerDes PCS 0 (QXGMII)			SerDes PCS 0 (QXGMII)
port 12: SerDes PCS 0 (QXGMII)			SerDes PCS 0 (QXGMII)
port 13: SerDes PCS 1				SerDes PCS 1
port 14: SerDes PCS 1 (QXGMII)			SerDes PCS 1 (QXGMII)
port 15: SerDes PCS 1 (QXGMII)			SerDes PCS 1 (QXGMII)
port 16: SerDes PCS 1 (QXGMII)			SerDes PCS 1 (QXGMII)

MaxLinear seems to tell board designers to always use port 9 as CPU port
and port 13 for an SFP cage or for an additional single-port PHY.
But this seem to be limitations of their current downstream driver
implementation and the hardware would be capable of using either
or both SerDes port as CPU port, and also support connecting a QXGMII PHY
to end up with 12 TP ports in total.

Port 0 indeed turned out to be the microcontroller, I can confirm that
because it screams when poking it with IEEE1588v2 packets, which you
get to see in the kernel logs:
net eth1: Invalid source port, packet dropped, tag: 88 c3 0f 04 00 00 00 10
                                                                         ^^
                                          IGP/EGP is 0 here, Record-ID is 1

The datasheet says that Record-ID "[...] is used for logging information
for PTP and OAM packets".

And calling mxl862xx_port_disable() on it makes it shut up...


Cheers


Daniel

Powered by blists - more mailing lists