Message-ID: <20260128205552.2mheraoxodwqisns@skbuf>
Date: Wed, 28 Jan 2026 22:55:52 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Daniel Golle <daniel@...rotopia.org>
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

On Wed, Jan 28, 2026 at 04:45:38PM +0000, Daniel Golle wrote:
> > > +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.

I think the next step is to figure out how to best allocate the
limited sub-interfaces for the CPU port depending on what is actually
needed from them, because...

> > 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?

...with multiple CPU ports, calls such as dsa_switch_for_each_cpu_port()
will enumerate them all. The second CPU port will exist and will be set
up like all others (including mxl862xx_configure_tag_proto()), it's just
that no user port will be mapped to it (dp->cpu_dp will all point to the
first CPU 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.

Ok, so in the next revision you'll set it to 17 according to the table
below, right?

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

For multi-port SerDes interfaces like 10G-QXGMII, technically each port
has its own PCS (port 10 -> PCS 1, port 11 -> PCS 2, etc). But I think I
understood the general idea. Maybe it would have been better to say
"SerDes lane 0 PCS 0" etc.

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

To be clear, what would be a proper name for the first column in this table?
Physical ports, right?

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

Ok, I assume that's what your board has as well. If I quickly run those
numbers into mxl862_tag_xmit(), then I guess that if you wanted to ping
each port (with cpu_port = 9), you'd set:

         MXL862_SUBIF_ID			MXL862_IGP_EGP
port 0:  7					9
port 1:  8					9
port 2:  9					9
port 3:  10					9
port 4:  11					9
port 5:  12					9
port 6:  13					9
port 7:  14					9
port 8:  15					9
port 9:  16					9
port 10: 17					9
port 11: 18					9
port 12: 19					9
port 13: 20					9
port 14: 21					9
port 15: 22					9
port 16: 23					9

I wonder how those MXL862_SUBIF_ID values map to the .number_of_ctp_port
configured by mxl862xx_configure_tag_proto(), which is 32 - DSA_MXL_PORT(9) == 22.
So to target port 16, you'd need a MXL862_SUBIF_ID which is out of range,
and results in an unrepresentable CTP index of 32. And subif_id values
0..6 are unused. Something doesn't add up...

Where does the 16 come from in the "usr_port + 16 - cpu_port" formula?

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

Interesting. If your host SoC supported 10G-QXGMII, you could
reconfigure both sides of that 10G SerDes interface on your existing
board and test quad CPU ports just for fun and giggles.

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

In SJA1110 I could also ping the microcontroller by putting an IP
address on that user port. I am shutting it down during probe time, in
sja1110_disable_microcontroller(), and not relying on the firmware at all.


Maybe this is a stupid question, but I still don't quite grasp the purpose
of the logical port + sub-interface scheme from its use.

>From what I could gather:
- xmit from Linux targets the logical port ID of the CPU port, and a
  sub-interface somehow in an implicit 1:1 association with user ports,
  which seems under-dimensioned in the maximal scenario
- the P-Mapper documentation says it can map one of {VLAN PCP, IP DSCP,
  LAG ID} to a sub-interface, but this feature is left to the firmware
  default state (MXL862XX_BRIDGE_PORT_CONFIG_MASK_EGRESS_CTP_MAPPING is
  never set), so I cannot make any assumptions about its influence

I don't quite understand how these two uses interact.

Powered by blists - more mailing lists