Re: [PATCH net-next v2 4/4] net: dsa: soce: Add basic support for SoC-e switch IP cores

[Vasilij Strassheim <v.strassheim@linutronix.de>]

On Tue, 2026-09-08 at 02:37 +0200, Andrew Lunn wrote:
> > +#define SOCE_CORE_VERSION_SHIFT			24
> > +#define SOCE_CORE_SUBVERSION_SHIFT		16
> > +#define SOCE_LICENSED_NUM_PORTS_MASK		GENMASK(31, 27)
> > +#define SOCE_IMPLEMENTED_NUM_PORTS_MASK		GENMASK(31, 27)
> > +#define SOCE_IMPLEMENTED_DSA			BIT(23)
> > +#define SOCE_DSA_REGS_BASE			0x1200
> > +#define SOCE_TAG_ALL_FRAMES_CTRL_OFFSET		(SOCE_DSA_REGS_BASE + 0x001c)
> > +#define SOCE_TAG_ALL_FRAMES_ENABLE		BIT(0)
> > +#define SOCE_CUSTOM_RULES_TAGGING_OFFSET	(SOCE_DSA_REGS_BASE + 0x0020)
> > +#define SOCE_CUSTOM_RULES_TAGGING_ENABLE	BIT(0)
> > +#define SOCE_MIN_CORE_VERSION			0x24
> > +#define SOCE_MIN_CORE_SUBVERSION		0x01
> 
> Could you fully describe the feature registers, so we have an idea
> what is actually there.
> 

Unfortunately, I cannot share the complete vendor documentation, but I
can provide an overview of the feature register fields and bits used by
the driver, as well as the surrounding registers and bits. This should
help to get an idea.
Each synthesized optional feature has its own register block. The
additional description focuses on the MDIO, and DSA blocks which are
relevant to this driver.

---
Global register block at 0x0000 (three registers currently used)
	0x00 Core Version - Major and minor revision number (RO)
	0x04 Licensed Features (RO)
	0x08 Reserved
	0x0C GUI Configuration - Implemented Features 0 (RO)
	0x10 GUI Configuration - Implemented Features 1 (RO)
	0x14 Reserved
	0x18 Node Address - MAC Address (31-0)
	0x1C Node Address - MAC Address (47-32)
	0x20 Node Address - IPv4 Address
	0x24 Node Address - IPv6 Address (31-0)
	0x28 Node Address - IPv6 Address (63-32)
	0x2C Node Address - IPv6 Address (95-64)
	0x30 Node Address - IPv6 Address (127-96)
	0x34 Switch Configuration - Maximum Frame Length
	0x38 Reserved
	0x3C Switch Configuration - Input Timestamp Addition
	0x40 Reserved
	0x44 Switch Configuration - Input Timestamp Removal
	0x48 Reserved
	0x4C Switch Configuration - Architecture Selection

	Core Version (RO)
	[31:24] - Major version number
	[23:16] - Minor version number
	[15:8] - Major revision number
	[7:0] - Minor revision number

	Licensed Features Register (RO) (Bits specify if the given
	feature is enabled by license):
	[31:27] - Maximum Number of Ports: These bits specify the maximum
	number of ports licensed
	[26:15] - Reserved
	- ACL: Access Control List (ACL) feature
	- LLDP: LLDP feature
	- IEEE 802.1X
	- IEEE 802.1AX-2008 - Link Aggregation
	- IEEE 802.3br/IEEE 802.1Qbu - Preemption
	- IEC 61158-4-2 - DLR
	- IEC 62439-2 - MRP
	- IGMP Snooping
	- Port mirroring
	- IEEE 802.1s - MSTP
	- IEEE 802.1w - RSTP
	- IEEE 1588 - Transparent Clock
	- Switch mode: managed or unmanaged
	- Secure Chip
	- Time Limitation

	Implemented Features 0 Register (RO, Default value from GUI,
	Bits specify if the given feature is synthesized):
	[31:27] - Number of Ports: These bits specify the number of
	ports implemented.
	- IEEE 802.1X
	- IEEE 802.1AX-2008 - Link Aggregation
	- IEEE 802.3br/IEEE 802.1Qbu - Preemption
	[23] - DSA: This bit specifies if the DSA functionality is
	implemented.
	- Port Mirroring:
	- IEEE 802.1s - MSTP
	- IEEE 802.1w - RSTP
	- MAC Level Ingress Frame Filtering
	- Egress Frame Rate Limiting
	- Egress Throughput Rate Limiting
	- MAC Ingress Frame Rate Limiting
	- Ingress Throughput Rate Limiting
	- IGMP Snooping
	- Multicast Filtering
	- MAC Table Type: (SVL/IVL/SVL+IVL)
	- Static Filtering Database
	- Port-based VLAN
	- IEEE 1588 - Transparent Clock Layer 3
	- IEEE 1588 - Transparent Clock P2P calculation
	- IEEE 1588 - Transparent Clock
	- Jumbo Frame
	- QoS - Priorities
	- Statistic Counters

	Implemented Features 1 Register (RO, Default value from GUI,
	Bits specify if the given feature is synthesized):
	- Redundancy Protocol (PRP/HSR/HSR+PRP)
	- IEC 61158-4-2 - DLR
	- IEC 62439-2 - MRP
	- ACL: Access Control List
	- LLDP

---
MDIO Bridge Registers, base address 0x0200
	0x00 MDIO - Control
	0x04 MDIO - Address / Device Type
	0x08 MDIO - Register Address / Write Data
	0x0C MDIO - Read Data

	MDIO - Control
	[26:16] - Bus selection: These bits specify the port number over
	which MDIO operation is performed.
	[4:3] - Transaction Type: These bits specify the type of MDIO
	transaction to be performed.
		0x0 - MDIO address operation (only valid when Clause 45
		is selected).
		0x1 - MDIO write operation.
		0x3 - MDIO read operation.
	[1] - MDIO Clause: If '0', Clause 22, if '1', Clause 45
	[0] - Operation Status: 0: Ready / 1: Operation in progress. Set
	to '1' to start a new operation.

	MDIO - Address / Device Type
	[12:8] - Register Address (if Clause 22)/Device Type (if Clause45)
	[4:0] - PHY Address: These bits specify the PHY address

	MDIO - Register Address / Write Data
	[15:0] - Register Address / Write Data: Depending on Clause 22/45
	and transaction type.

	MDIO - Read Data
	[15:0] - Read Data: Data sampled after a MDIO read transaction.

---
DSA Frame Tagging Registers, base address 0x1200 (Mostly R/W access if
the feature is available. Default values 0 or from synthesis GUI.)
	0x00 CPU Ports (RO)
	0x04 DSA Predefined Rules - Control
	0x08 DSA Predefined Rules - Index
	0x0C DSA Predefined Rules - CPU Port List
	0x10-0x18 Reserved
	0x1C DSA Tag All - Control ([0] - If '1', all frames
	transmitted to the CPU port(s) will be DSA tagged. If '0', only
	those frames identified as DSA will be tagged.)
	0x20 DSA Custom Rules - Enable ([0] - Globally enable or
	disable the Custom DSA Tagging functionality)
	0x24 DSA Custom Rules - Control
	0x28 DSA Custom Rules - Index
	0x2C DSA Custom Rules - Destination MAC Address LO
	0x30 DSA Custom Rules - Destination MAC Address HI
	0x34 DSA Custom Rules - Ethertype
	0x38 DSA Custom Rules - Parameter Mask
	0x3C DSA Custom Rules - CPU Port List
	0x40 DSA Custom Rules - Trap/Mirror

While preparing this description I found a mismatch with MDIO Bus
selection Bits 26:16 in code. I will fix this for next version.


> > +struct soce_probe_desc {
> > +	u32 core_version_offset;
> > +	u32 licensed_features_offset;
> > +	u32 implemented_features0_offset;
> > +	u32 mdio_master_offset;
> > +};
> > +
> > +static const struct soce_probe_desc soce_probe_desc_swip_00_04_0c_10 = {
> > +	.core_version_offset		= 0x0000,
> > +	.licensed_features_offset	= 0x0004,
> > +	.implemented_features0_offset	= 0x000c,
> > +	.mdio_master_offset		= 0x0200,
> > +};
> 
> How fixed/variable are these? I'm just thinking there may be too much
> abstraction here. To some extent, we leave abstractions out until they
> are needed. If you know there are other devices out there which have
> these at other addresses, then O.K. But if not, i would keep it KISS.
> 

Right. I introduced the descriptor to make potential future
register-layout variants easier to support, but there is currently no
concrete variant that requires it.
Will fix this.

> > +static int soce_sw_detect_features(struct soce_dsa_local *local,
> > +				   const struct soce_probe_desc *probe_desc,
> > +				   u32 *numports)
> > +{
> > +	u32 implemented_numports;
> > +	u32 licensed_numports;
> > +	u32 regval;
> > +
> > +	regval = readl(local->base_addr + probe_desc->licensed_features_offset);
> > +	licensed_numports = FIELD_GET(SOCE_LICENSED_NUM_PORTS_MASK, regval);
> > +	if (!licensed_numports || licensed_numports > SOCE_MAX_NUM_PORTS)
> > +		return -EINVAL;
> > +
> > +	regval = readl(local->base_addr +
> > +		       probe_desc->implemented_features0_offset);
> > +	if (!(regval & SOCE_IMPLEMENTED_DSA))
> > +		return -ENODEV;
> 
> I find it useful to have macros like SOCE_IMPLEMENTED_DSA indicate
> they apply to features0. It makes it easier to spot dumb typos when
> you apply it to features1, not features0.
> 

Agreed. I will do it for the next version.

> I also wounder at the name. Does the data sheet really call it DSA?
> 

Yes, there is even a chapter describing Distributed Switch Architecture
(DSA) as "a subsystem that was primarily designed to support specific
vendor switches using Linux, ..." followed by information about "custom
DSA tagging".

> > +
> > +	implemented_numports = FIELD_GET(SOCE_IMPLEMENTED_NUM_PORTS_MASK,
> > +					 regval);
> > +	if (!implemented_numports ||
> > +	    implemented_numports > licensed_numports)
> > +		return -EINVAL;
> > +
> > +	*numports = implemented_numports;
> 
> How is this going to scale when you need to look at more bits in these
> registers? It seems like at some point you are going to need to pass a
> structure to be filled in.
> 

Yes, I agree that a feature structure will be needed once more fields
are used. I would prefer to keep the current interface for now and
introduce struct soce_features when another detected feature needs to be
retained. At the moment, it would contain only a single member.

> > +static int soce_register_mdio_bus(struct soce_priv *priv, struct device *dev,
> > +				  struct device_node *mdio_np,
> > +				  u32 mdio_output)
> > +{
> > +	struct soce_mdio_bus *state;
> > +	struct mii_bus *bus;
> > +
> > +	bus = devm_mdiobus_alloc(dev);
> > +	if (!bus)
> > +		return -ENOMEM;
> > +
> > +	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
> > +	if (!state)
> > +		return -ENOMEM;
> > +
> > +	state->ds = priv->ds;
> > +	state->mdio_output = mdio_output;
> > +
> > +	bus->priv = state;
> > +	bus->name = "soce mdio";
> > +	bus->read = soce_mdio_read;
> > +	bus->write = soce_mdio_write;
> > +	bus->read_c45 = soce_mdio_read_c45;
> > +	bus->write_c45 = soce_mdio_write_c45;
> > +	/* ds->dst can be NULL during probe, before dsa_register_switch() */
> > +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mdio-%u", dev_name(dev),
> > +		 mdio_output);
> 
> Why the comment. How is it relevant?

I missed to update or remove comments here and on other places while
rewriting the code. I will fix all of them for next version.

> 
> > +static int soce_mdio_23_02_read_c22(struct dsa_switch *ds, int mdio_output,
> > +				    int phy_addr, int regnum)
> > +{
> > +	void __iomem *ctrl, *params, *read_reg;
> > +	struct soce_priv *priv = ds->priv;
> > +	struct soce_dsa_local *local;
> > +	u32 regvalue;
> > +	int ret;
> > +
> > +	local = &priv->local;
> > +	ctrl = local->mdio_master_addr + SOCE_MDIO_CTRL_OFFSET;
> > +	params = local->mdio_master_addr + SOCE_MDIO_23_02_PARAMS_OFFSET;
> > +	read_reg = local->mdio_master_addr + SOCE_MDIO_23_02_READ_OFFSET;
> > +
> > +	regvalue = (regnum << SOCE_MDIO_23_02_CTRL_REGADDRDEVTYPE_OFFSET) +
> > +		   (phy_addr << SOCE_MDIO_23_02_CTRL_PHYADDR_OFFSET);
> > +	writel(regvalue, params);
> > +
> > +	regvalue = ((mdio_output << SOCE_MDIO_23_02_CTRL_BUS_OFFSET) +
> > +		    (0x3 << SOCE_MDIO_23_02_CTRL_TRANSTYPE_OFFSET) +
> 
> 0x3 means read?
> 
> > +		    (0x0 << SOCE_MDIO_23_02_CTRL_CLAUSE_OFFSET) +
> 
> 0x0 means C22
> 
> > +		    (0x1 << SOCE_MDIO_23_02_CTRL_OPSTATUS_OFFSET));
> 
> What does 0x1 mean? Please add some #defines for theses to act as
> documentation.

This code will be replaced as part of mentioned MDIO controller and mux
rework. I will address this there by adding descriptive definitions.

> 
> 	Andrew

Thanks,
Vasilij