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

[Andrew Lunn]

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

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

> +/* 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.

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

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

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

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

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

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.

	Andrew