Re: [RFC PATCH v11 net-next 05/10] net: dsa: microchip: add DSA support for microchip lan937x

[Vladimir Oltean]

On Fri, Mar 25, 2022 at 10:23:36PM +0530, Prasanna Vengateshan wrote:
> +static void lan937x_r_mib_stats64(struct ksz_device *dev, int port)
> +{
> +	struct ksz_port_mib *mib = &dev->ports[port].mib;
> +	struct rtnl_link_stats64 *s;
> +	u64 *ctr = mib->counters;
> +
> +	s = &mib->stats64;
> +	spin_lock(&mib->stats64_lock);

I haven't looked at further patches yet to see if the situation improves
or not, but right now, this spin lock is useless, as you do not
implement .get_stats64.

> +
> +	s->rx_packets = ctr[lan937x_mib_rx_mcast] +
> +			ctr[lan937x_mib_rx_bcast] +
> +			ctr[lan937x_mib_rx_ucast] +
> +			ctr[lan937x_mib_rx_pause];
> +
> +	s->tx_packets = ctr[lan937x_mib_tx_mcast] +
> +			ctr[lan937x_mib_tx_bcast] +
> +			ctr[lan937x_mib_tx_ucast] +
> +			ctr[lan937x_mib_tx_pause];
> +
> +	s->rx_bytes = ctr[lan937x_mib_rx_total];
> +	s->tx_bytes = ctr[lan937x_mib_tx_total];
> +
> +	s->rx_errors = ctr[lan937x_mib_rx_fragments] +
> +		       ctr[lan937x_mib_rx_jabbers] +
> +		       ctr[lan937x_mib_rx_sym_err] +
> +		       ctr[lan937x_mib_rx_align_err] +
> +		       ctr[lan937x_mib_rx_crc_err];
> +
> +	s->tx_errors = ctr[lan937x_mib_tx_exc_col] +
> +		       ctr[lan937x_mib_tx_late_col];
> +
> +	s->rx_dropped = ctr[lan937x_mib_rx_discard];
> +	s->tx_dropped = ctr[lan937x_mib_tx_discard];
> +	s->multicast = ctr[lan937x_mib_rx_mcast];
> +
> +	s->collisions = ctr[lan937x_mib_tx_late_col] +
> +			ctr[lan937x_mib_tx_single_col] +
> +			ctr[lan937x_mib_tx_mult_col];
> +
> +	s->rx_length_errors = ctr[lan937x_mib_rx_fragments] +
> +			      ctr[lan937x_mib_rx_jabbers];
> +
> +	s->rx_crc_errors = ctr[lan937x_mib_rx_crc_err];
> +	s->rx_frame_errors = ctr[lan937x_mib_rx_align_err];
> +	s->tx_aborted_errors = ctr[lan937x_mib_tx_exc_col];
> +	s->tx_window_errors = ctr[lan937x_mib_tx_late_col];
> +
> +	spin_unlock(&mib->stats64_lock);
> +}

> +static int lan937x_init(struct ksz_device *dev)
> +{
> +	int ret;
> +
> +	ret = lan937x_switch_init(dev);
> +	if (ret < 0) {
> +		dev_err(dev->dev, "failed to initialize the switch");
> +		return ret;
> +	}
> +
> +	/* enable Indirect Access from SPI to the VPHY registers */
> +	ret = lan937x_enable_spi_indirect_access(dev);

Do you need to call this both from lan937x_init() and from lan937x_setup()?

> +	if (ret < 0) {
> +		dev_err(dev->dev, "failed to enable spi indirect access");
> +		return ret;
> +	}
> +
> +	ret = lan937x_mdio_register(dev);
> +	if (ret < 0) {
> +		dev_err(dev->dev, "failed to register the mdio");
> +		return ret;
> +	}
> +
> +	return 0;
> +}

> +static void lan937x_port_stp_state_set(struct dsa_switch *ds, int port,
> +				       u8 state)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_port *p;
> +	u8 data;
> +
> +	lan937x_pread8(dev, port, P_STP_CTRL, &data);

This is a copy-paste of ksz8_port_stp_state_set() except for the use of
lan937x_pread8() instead of ksz_pread8(). But ksz_pread8() should work
too, since it calls dev->dev_ops->get_port_addr(port, offset) which you
translate into PORT_CTRL_ADDR(port, offset) which is exactly what
lan937x_pread8() does.

> +	data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE | PORT_LEARN_DISABLE);
> +
> +	switch (state) {
> +	case BR_STATE_DISABLED:
> +		data |= PORT_LEARN_DISABLE;
> +		break;
> +	case BR_STATE_LISTENING:
> +		data |= (PORT_RX_ENABLE | PORT_LEARN_DISABLE);
> +		break;
> +	case BR_STATE_LEARNING:
> +		data |= PORT_RX_ENABLE;
> +		break;
> +	case BR_STATE_FORWARDING:
> +		data |= (PORT_TX_ENABLE | PORT_RX_ENABLE);
> +		break;
> +	case BR_STATE_BLOCKING:
> +		data |= PORT_LEARN_DISABLE;
> +		break;
> +	default:
> +		dev_err(ds->dev, "invalid STP state: %d\n", state);
> +		return;
> +	}
> +
> +	lan937x_pwrite8(dev, port, P_STP_CTRL, data);
> +
> +	p = &dev->ports[port];
> +	p->stp_state = state;
> +
> +	ksz_update_port_member(dev, port);
> +}