Re: [PATCH 4/4] net: dsa: soce: Add initial driver support for MRS switches

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

On Wed, 2026-07-29 at 22:05 +0200, Andrew Lunn wrote:
> > +static u8 soce_map_stp_state(u8 state)
> > +{
> > +	switch (state) {
> > +	case BR_STATE_DISABLED:
> > +	case BR_STATE_BLOCKING:
> > +	case BR_STATE_LISTENING:
> > +		return 0;
> > +	case BR_STATE_LEARNING:
> > +		return 1;
> > +	case BR_STATE_FORWARDING:
> > +	default:
> > +		return 3;
> 
> #defines for 0, 1 and 3.
> 
> What does 2 mean?
> 
> Since you don't implement bridge offload, i'm not sure having STP
> states makes any sense.

I tested bridge and STP and observed the callback calls with something like
this (reduced setup):
# ip link add name br0 type bridge
# ip link set dev port0 master br0
# ip link set dev port1 master br0
# brctl stp br0 on

That probably wasn't enough. I'll take a closer look at it and either fix it
or remove it for the next version.

> 
> > +static void soce_fast_age(struct dsa_switch *ds, int port,
> > +			  const struct soce_layout *layout)
> > +{
> > +	struct soce_priv *priv = ds->priv;
> > +	struct soce_dsa_local *local;
> > +	void __iomem *p_mes_ctrl;
> > +	u32 val;
> 
> Does aging make any sense without bridge offload?

Same as above. I'll take a closer look at it and either fix it or remove it
for the next version.

> 
> > +/* Bridge membership is handled by the switch forwarding model directly. */
> > +int soce_port_bridge_join(struct dsa_switch *ds, int port,
> > +			  struct dsa_bridge bridge, bool *tx_fwd_offload,
> > +			  struct netlink_ext_ack *extack)
> > +{
> > +	return 0;
> > +}
> > +
> > +void soce_port_bridge_leave(struct dsa_switch *ds, int port,
> > +			    struct dsa_bridge bridge)
> > +{
> > +}
> 
> If you don't support bridge offload, you should not need these.

Also here. I'll take a closer look at it and either fix it or remove it for
the next version.

> 
> > +static void soce_phylink_get_caps(struct dsa_switch *ds, int port,
> > +				  struct phylink_config *config)
> > +{
> > +	if (dsa_is_cpu_port(ds, port)) {
> > +		__set_bit(PHY_INTERFACE_MODE_GMII,
> 
> Is that really true? What is part of the IP core, and what is to do
> with integration of the core into the system as a whole? It seems like
> RGMII is also supported, so why cannot i instantiate a RGMII MII blob
> for the CPU port as well?

No it's wrong, I will adjust that.

> 
> > +			  config->supported_interfaces);
> > +		config->mac_capabilities = MAC_10 | MAC_100 | MAC_1000 |
> > +					   MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
> > +		return;
> > +	}
> > +
> > +	phy_interface_set_rgmii(config->supported_interfaces);
> 
> And flipping that around, if the core supports GMII, could i
> instantiate a GMII user port?

Will also be fixed in the next version.

> 
> > +static const struct dsa_switch_ops soce_switch_ops = {
> > +	.get_tag_protocol	= soce_get_tag_protocol,
> > +	.setup			= soce_setup,
> > +	.teardown		= soce_teardown,
> > +	.phylink_get_caps	= soce_phylink_get_caps,
> > +	.phy_read		= soce_phy_read,
> > +	.phy_write		= soce_phy_write,
> 
> phy_read and phy_write are legacy. Instantiate a Linux MDIO bus and
> don't list anything here.

I will do it.

> 
> > +static const struct soce_variant_desc *
> > +soce_match_variant(const char *compatible)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; soce_variants[i].compatible; i++) {
> > +		if (!strcmp(compatible, soce_variants[i].compatible))
> > +			return &soce_variants[i];
> > +	}
> > +
> > +	return NULL;
> 
> This kind of looks like a reinvention of of_device_get_match_data()

I will update this in the next version.

> 
> > +static int soce_sw_parse_port_mdio_config(struct soce_dsa_local *local,
> > +					  struct device *dev,
> > +					  struct device_node *port_node,
> > +					  u32 port)
> > +{
> > +	struct device_node *phy_node;
> > +	u32 val;
> > +	int ret;
> > +
> > +	if (of_find_property(port_node, "ethernet", NULL) ||
> > +	    of_find_property(port_node, "link", NULL)) {
> > +		if (of_find_property(port_node, "phy-handle", NULL)) {
> > +			dev_err(dev,
> > +				"phy-handle not allowed on CPU/DSA port %u\n",
> > +				port);
> > +			return -EINVAL;
> > +		}
> > +
> > +		return 0;
> > +	}
> 
> Why is all of this parsing of DT needed? These should all be standard
> properties which the DSA core handles.

I will make greater adjustments to the driver here and in other places for
the next version.

> 
> This currently has the feel of being a "vendor crap" driver. Please
> spend some time to clean this driver up, bring it up to Mainline
> standards.

This is actually already the cleaner version.
Thank you for the feedback. This will allow me to reorganize the project
accordingly and adapt the code more closely to already mainlined drivers.
It looks like Lantiq provides a good example.

> 
> 	Andrew

Thanks,
Vasilij