Re: [PATCH net-next 2/3] net: dsa: add Arrow SpeedChips XRS700x driver

Hi George,

On Fri, Nov 20, 2020 at 12:16:26PM -0600, George McCollister wrote:

Add a driver with initial support for the Arrow SpeedChips XRS7000
series of gigabit Ethernet switch chips which are typically used in
critical networking applications.

The switches have up to three RGMII ports and one RMII port.
Management to the switches can be performed over i2c or mdio.

Support for advanced features such as PTP and
HSR/PRP (IEC 62439-3 Clause 5 & 4) is not included in this patch and
may be added at a later date.

Signed-off-by: George McCollister <george.mccollister@gmail.com>
---
 drivers/net/dsa/Kconfig        |  26 ++
 drivers/net/dsa/Makefile       |   3 +
 drivers/net/dsa/xrs700x.c      | 529 +++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/xrs700x.h      |  27 +++
 drivers/net/dsa/xrs700x_i2c.c  | 148 ++++++++++++
 drivers/net/dsa/xrs700x_mdio.c | 160 +++++++++++++
 drivers/net/dsa/xrs700x_reg.h  | 205 ++++++++++++++++

How much code do you plan to add to this driver? If it's going to
include IEEE 1588 and HSR/PRP offloading, would it make sense to put its
source code in a new folder now, to avoid doing that later?

Uh-oh, Jakub might not like these RMON counters being exposed to
ethtool. See:
https://patchwork.kernel.org/project/netdevbpf/patch/20201115073533.1366-1-o.rempel@pengutronix.de/

+	{XRS_RX_HSR_PRP_L(0), "rx_hsr_prp"},
+	{XRS_RX_WRONGLAN_L(0), "rx_wronglan"},
+	{XRS_RX_DUPLICATE_L(0), "rx_duplicate"},
+	{XRS_TX_OCTETS_L(0), "tx_octets"},
+	{XRS_TX_UNICAST_L(0), "tx_unicast"},
+	{XRS_TX_BROADCAST_L(0), "tx_broadcast"},
+	{XRS_TX_MULTICAST_L(0), "tx_multicast"},
+	{XRS_TX_HSR_PRP_L(0), "tx_hsr_prp"},
+	{XRS_PRIQ_DROP_L(0), "priq_drop"},
+	{XRS_EARLY_DROP_L(0), "early_drop"},
+};
+
+static void xrs700x_get_strings(struct dsa_switch *ds, int port,
+				u32 stringset, uint8_t *data)
+{
+	int i;
+
+	if (stringset != ETH_SS_STATS)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(xrs700x_mibs); i++) {
+		strlcpy(data, xrs700x_mibs[i].name, ETH_GSTRING_LEN);
+		data += ETH_GSTRING_LEN;
+	}
+}
+
+static int xrs700x_get_sset_count(struct dsa_switch *ds, int port, int sset)
+{
+	if (sset != ETH_SS_STATS)
+		return -EOPNOTSUPP;
+
+	return ARRAY_SIZE(xrs700x_mibs);
+}
+
+static void xrs700x_read_port_counters(struct xrs700x *priv, int port)
+{
+	int i;
+	struct xrs700x_port *p = &priv->ports[port];
+
+	mutex_lock(&p->mib_mutex);
+
+	/* Capture counter values */
+	regmap_write(priv->regmap, XRS_CNT_CTRL(port), 1);
+
+	for (i = 0; i < ARRAY_SIZE(xrs700x_mibs); i++) {
+		unsigned int high = 0, low = 0, reg;
+
+		reg = xrs700x_mibs[i].offset + XRS_PORT_OFFSET * port;
+		regmap_read(priv->regmap, reg, &low);
+		regmap_read(priv->regmap, reg + 2, &high);
+
+		p->mib_data[i] += (high << 16) | low;
+	}
+
+	mutex_unlock(&p->mib_mutex);
+}
+
+static void xrs700x_mib_work(struct work_struct *work)
+{
+	struct xrs700x *priv = container_of(work, struct xrs700x,
+					    mib_work.work);
+	int i;
+
+	for (i = 0; i < priv->ds->num_ports; i++)
+		xrs700x_read_port_counters(priv, i);
+
+	schedule_delayed_work(&priv->mib_work, XRS700X_MIB_INTERVAL);
+}
+
+static void xrs700x_get_ethtool_stats(struct dsa_switch *ds, int port,
+				      uint64_t *data)
+{
+	struct xrs700x *priv = ds->priv;
+	struct xrs700x_port *p = &priv->ports[port];
+
+	xrs700x_read_port_counters(priv, port);
+
+	mutex_lock(&p->mib_mutex);
+	memcpy(data, p->mib_data, sizeof(*data) * ARRAY_SIZE(xrs700x_mibs));
+	mutex_unlock(&p->mib_mutex);
+}
+
+static int xrs700x_setup_regmap_range(struct xrs700x *priv)
+{
+	struct reg_field ps_forward = REG_FIELD_ID(XRS_PORT_STATE(0), 0, 1,
+						   priv->ds->num_ports,
+						   XRS_PORT_OFFSET);

Oh, hey, another REG_FIELD_ID user!

+	struct reg_field ps_management = REG_FIELD_ID(XRS_PORT_STATE(0), 2, 3,
+						      priv->ds->num_ports,
+						      XRS_PORT_OFFSET);
+	struct reg_field ps_sel_speed = REG_FIELD_ID(XRS_PORT_STATE(0), 4, 9,
+						     priv->ds->num_ports,
+						     XRS_PORT_OFFSET);
+	struct reg_field ps_cur_speed = REG_FIELD_ID(XRS_PORT_STATE(0), 10, 11,
+						     priv->ds->num_ports,
+						     XRS_PORT_OFFSET);
+
+	priv->ps_forward = devm_regmap_field_alloc(priv->dev, priv->regmap,
+						   ps_forward);
+	if (IS_ERR(priv->ps_forward))
+		return PTR_ERR(priv->ps_forward);
+
+	priv->ps_management = devm_regmap_field_alloc(priv->dev, priv->regmap,
+						      ps_management);
+	if (IS_ERR(priv->ps_management))
+		return PTR_ERR(priv->ps_management);
+
+	priv->ps_sel_speed = devm_regmap_field_alloc(priv->dev, priv->regmap,
+						     ps_sel_speed);
+	if (IS_ERR(priv->ps_sel_speed))
+		return PTR_ERR(priv->ps_sel_speed);
+
+	priv->ps_cur_speed = devm_regmap_field_alloc(priv->dev, priv->regmap,
+						     ps_cur_speed);
+	if (IS_ERR(priv->ps_cur_speed))
+		return PTR_ERR(priv->ps_cur_speed);

Should you try to automate allocating these? You might get tired of
adding and adding and adding to this function really quick. You might
get some inspiration from ocelot_regfields_init() and that driver's use
of regmap in general.

+
+	return 0;
+}
+
+static enum dsa_tag_protocol xrs700x_get_tag_protocol(struct dsa_switch *ds,
+						      int port,
+						      enum dsa_tag_protocol m)
+{
+	return DSA_TAG_PROTO_XRS700X;
+}
+
+static int xrs700x_reset(struct dsa_switch *ds)
+{
+	struct xrs700x *priv = ds->priv;
+	int ret;
+	unsigned int val;
+
+	ret = regmap_write(priv->regmap, XRS_GENERAL, XRS_GENERAL_RESET);
+	if (ret)
+		goto error;
+
+	ret = regmap_read_poll_timeout(priv->regmap, XRS_GENERAL,
+				       val, !(val & XRS_GENERAL_RESET),
+				       10, 1000);
+error:
+	if (ret) {
+		dev_err_ratelimited(priv->dev, "error resetting switch: %d\n",
+				    ret);
+	}
+
+	return ret;
+}
+
+static void xrs700x_port_stp_state_set(struct dsa_switch *ds, int port,
+				       u8 state)
+{
+	struct xrs700x *priv = ds->priv;
+	unsigned int val;
+
+	switch (state) {
+	case BR_STATE_DISABLED:
+		val = XRS_PORT_DISABLED;
+		break;
+	case BR_STATE_LISTENING:
+		val = XRS_PORT_DISABLED;
+		break;
+	case BR_STATE_LEARNING:
+		val = XRS_PORT_LEARNING;
+		break;
+	case BR_STATE_FORWARDING:
+		val = XRS_PORT_FORWARDING;
+		break;
+	case BR_STATE_BLOCKING:
+		val = XRS_PORT_DISABLED;

Why not just put BR_STATE_DISABLED and BR_STATE_BLOCKING one under the
other?

	case BR_STATE_BLOCKING:
	case BR_STATE_DISABLED:
		val = XRS_PORT_DISABLED;

+		break;
+	default:
+		dev_err(ds->dev, "invalid STP state: %d\n", state);
+		return;
+	}
+
+	regmap_fields_write(priv->ps_forward, port, val);
+
+	dev_dbg_ratelimited(priv->dev, "%s - port: %d, state: %u, val: 0x%x\n",
+			    __func__, port, state, val);
+}
+
+static int xrs700x_port_setup(struct dsa_switch *ds, int port)
+{
+	struct xrs700x *priv = ds->priv;
+	bool cpu_port = dsa_is_cpu_port(ds, port);

Reverse Christmas tree notation please.

+	unsigned int val;

Ugh, you couldn't have initialized this with zero here? It looks ugly
putting that in the for loop.

+	int ret, i;
+
+	xrs700x_port_stp_state_set(ds, port, BR_STATE_DISABLED);
+
+	/* Disable forwarding to non-CPU ports */
+	for (val = 0, i = 0; i < ds->num_ports; i++) {
+		if (!dsa_is_cpu_port(ds, i))
+			val |= BIT(i);
+	}
+
+	ret = regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port), val);
+	if (ret)
+		return ret;
+
+	val = cpu_port ? XRS_PORT_MODE_MANAGEMENT : XRS_PORT_MODE_NORMAL;
+	ret = regmap_fields_write(priv->ps_management, port, val);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int xrs700x_setup(struct dsa_switch *ds)
+{
+	struct xrs700x *priv = ds->priv;
+	int ret, i;
+
+	ret = xrs700x_reset(ds);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < ds->num_ports; i++) {
+		ret = xrs700x_port_setup(ds, i);
+		if (ret)
+			return ret;
+	}
+
+	schedule_delayed_work(&priv->mib_work, XRS700X_MIB_INTERVAL);
+
+	return 0;
+}
+
+static void xrs700x_phylink_validate(struct dsa_switch *ds, int port,
+				     unsigned long *supported,
+				     struct phylink_link_state *state)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+
+	switch (port) {
+	case 0:
+		break;
+	case 1:
+	case 2:
+	case 3:
+		phylink_set(mask, 1000baseT_Full);
+		break;
+	default:
+		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
+		dev_err(ds->dev, "Unsupported port: %i\n", port);
+		return;
+	}
+
+	phylink_set_port_modes(mask);
+
+	/* The switch only supports full duplex. */
+	phylink_set(mask, 10baseT_Full);
+	phylink_set(mask, 100baseT_Full);
+
+	bitmap_and(supported, supported, mask,
+		   __ETHTOOL_LINK_MODE_MASK_NBITS);
+	bitmap_and(state->advertising, state->advertising, mask,
+		   __ETHTOOL_LINK_MODE_MASK_NBITS);
+}
+
+static void xrs700x_phylink_mac_config(struct dsa_switch *ds, int port,
+				       unsigned int mode,
+				       const struct phylink_link_state *state)

As far as I understand phylink, you should be programming the link speed
of the RGMII/RMII MAC from the .mac_link_up callback.

+{
+	struct xrs700x *priv = ds->priv;
+	unsigned int val;
+
+	switch (state->speed) {
+	case SPEED_1000:
+		val = XRS_PORT_SPEED_1000;
+		break;
+	case SPEED_100:
+		val = XRS_PORT_SPEED_100;
+		break;
+	case SPEED_10:
+		val = XRS_PORT_SPEED_10;
+		break;
+	default:
+		return;
+	}
+
+	regmap_fields_write(priv->ps_sel_speed, port, val);
+
+	dev_dbg_ratelimited(priv->dev, "%s: port: %d mode: %u speed: %u\n",
+			    __func__, port, mode, state->speed);
+}
+
+static int xrs700x_bridge_join(struct dsa_switch *ds, int port,
+			       struct net_device *bridge)
+{
+	struct xrs700x *priv = ds->priv;
+	unsigned int i, ret, mask = 0;
+
+	for (i = 0; i < ds->num_ports; i++) {
+		if (dsa_to_port(ds, i)->bridge_dev == bridge ||
+		    dsa_is_cpu_port(ds, i))
+			continue;
+
+		mask |= BIT(i);
+	}
+
+	dev_dbg(priv->dev, "%s: port: %d mask: 0x%x\n", __func__,
+		port, mask);
+
+	for (i = 0; i < ds->num_ports; i++) {
+		if (dsa_to_port(ds, i)->bridge_dev != bridge)
+			continue;
+
+		ret = regmap_write(priv->regmap, XRS_PORT_FWD_MASK(i), mask);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static void xrs700x_bridge_leave(struct dsa_switch *ds, int port,
+				 struct net_device *bridge)

Don't be lazy, you can avoid copy-pasting the implementation for this
one...

+{
+	struct xrs700x *priv = ds->priv;
+	unsigned int i, cpu_mask = 0, mask = 0;
+
+	for (i = 0; i < ds->num_ports; i++) {
+		if (dsa_is_cpu_port(ds, i))
+			continue;
+
+		cpu_mask |= BIT(i);
+
+		if (dsa_to_port(ds, i)->bridge_dev == bridge)
+			continue;
+
+		mask |= BIT(i);
+	}
+
+	dev_dbg(priv->dev, "%s: port: %d mask: 0x%x\n", __func__,
+		port, mask);
+
+	for (i = 0; i < ds->num_ports; i++) {
+		if (dsa_to_port(ds, i)->bridge_dev != bridge)
+			continue;
+
+		regmap_write(priv->regmap, XRS_PORT_FWD_MASK(i), mask);
+	}
+
+	regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port), cpu_mask);
+}
+
+static const struct dsa_switch_ops xrs700x_ops = {
+	.get_tag_protocol	= xrs700x_get_tag_protocol,
+	.setup			= xrs700x_setup,
+	.port_stp_state_set	= xrs700x_port_stp_state_set,
+	.phylink_validate	= xrs700x_phylink_validate,
+	.phylink_mac_config	= xrs700x_phylink_mac_config,
+	.get_strings		= xrs700x_get_strings,
+	.get_sset_count		= xrs700x_get_sset_count,
+	.get_ethtool_stats	= xrs700x_get_ethtool_stats,
+	.port_bridge_join	= xrs700x_bridge_join,
+	.port_bridge_leave	= xrs700x_bridge_leave,
+};
+
+static int xrs700x_detect(struct xrs700x *dev)
+{
+	int i, ret;
+	unsigned int id;
+
+	ret = regmap_read(dev->regmap, XRS_DEV_ID0, &id);
+	if (ret) {
+		dev_err(dev->dev, "error %d while reading switch id.\n",
+			ret);
+		return ret;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(xrs700x_models); i++) {
+		if (xrs700x_models[i].id == id) {
+			dev->ds->num_ports = xrs700x_models[i].num_ports;
+			dev_info(dev->dev, "%s detected.\n",
+				 xrs700x_models[i].name);
+			return 0;
+		}
+	}
+
+	dev_err(dev->dev, "unknown switch id 0x%x.\n", id);
+
+	return -ENODEV;
+}
+
+struct xrs700x *xrs700x_switch_alloc(struct device *base, void *priv)
+{
+	struct dsa_switch *ds;
+	struct xrs700x *dev;
+
+	ds = devm_kzalloc(base, sizeof(*ds), GFP_KERNEL);
+	if (!ds)
+		return NULL;
+
+	ds->dev = base;
+	ds->num_ports = DSA_MAX_PORTS;

Why so many?

Please sort variable declaration in the order of decreasing line length.

+
+	buf[0] = reg >> 23 & 0xff;
+	buf[1] = reg >> 15 & 0xff;
+	buf[2] = reg >> 7 & 0xff;
+	buf[3] = (reg & 0x7f) << 1;
+
+	ret = i2c_master_send(i2c, buf, sizeof(buf));
+	if (ret < 0) {
+		dev_err(dev, "xrs i2c_master_send returned %d\n", ret);
+		return ret;
+	}
+
+	ret = i2c_master_recv(i2c, buf, 2);
+	if (ret < 0) {
+		dev_err(dev, "xrs i2c_master_recv returned %d\n", ret);
+		return ret;
+	}
+
+	*val = buf[0] << 8 | buf[1];
+
+	return 0;
+}