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

[Vladimir Oltean <olteanv@gmail.com>]

On Wed, Jan 13, 2021 at 08:59:21AM -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>
> ---

Some non-exhaustive feedback below.

> +static void xrs700x_teardown(struct dsa_switch *ds)
> +{
> +	struct xrs700x *priv = ds->priv;
> +
> +	cancel_delayed_work_sync(&priv->mib_work);
> +}
> +

> +static void xrs700x_mac_link_up(struct dsa_switch *ds, int port,
> +				unsigned int mode, phy_interface_t interface,
> +				struct phy_device *phydev,
> +				int speed, int duplex,
> +				bool tx_pause, bool rx_pause)
> +{
> +	struct xrs700x *priv = ds->priv;
> +	unsigned int val;
> +
> +	switch (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, speed);
> +}

What PHY interface types does the switch support as of this patch?
No RGMII delay configuration needed?

> +
> +static int xrs700x_bridge_common(struct dsa_switch *ds, int port,
> +				 struct net_device *bridge, bool join)
> +{
> +	unsigned int i, cpu_mask = 0, mask = 0;
> +	struct xrs700x *priv = ds->priv;
> +	int ret;
> +
> +	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);
> +	}
> +
> +	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);

Maybe it would be worth mentioning in a comment that PORT_FWD_MASK's
encoding is "1 = Disable forwarding to the port", otherwise this is
confusing.

> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!join) {
> +		ret = regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port),
> +				   cpu_mask);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

> +static int xrs700x_detect(struct xrs700x *dev)
> +{
> +	const struct xrs700x_info *info;
> +	unsigned int id;
> +	int ret;
> +
> +	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;
> +	}
> +
> +	info = of_device_get_match_data(dev->dev);
> +	if (!info)
> +		return -EINVAL;
> +
> +	if (info->id == id) {
> +		dev->ds->num_ports = info->num_ports;
> +		dev_info(dev->dev, "%s detected.\n", info->name);
> +		return 0;
> +	}
> +
> +	dev_err(dev->dev, "expected switch id 0x%x but found 0x%x.\n",
> +		info->id, id);

I've been there too, not the smartest of decisions in the long run. See
commit 0b0e299720bb ("net: dsa: sja1105: use detected device id instead
of DT one on mismatch") if you want a sneak preview of how this is going
to feel two years from now. If you can detect the device id you're
probably better off with a single compatible string.

> +
> +	return -ENODEV;
> +}
> +
> +static int xrs700x_alloc_port_mib(struct xrs700x *dev, int port)
> +{
> +	struct xrs700x_port *p = &dev->ports[port];
> +	size_t mib_size = sizeof(*p->mib_data) * ARRAY_SIZE(xrs700x_mibs);

Reverse Christmas tree ordering... sorry.

> +int xrs700x_switch_register(struct xrs700x *dev)
> +{
> +	int ret;
> +	int i;
> +
> +	ret = xrs700x_detect(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = xrs700x_setup_regmap_range(dev);
> +	if (ret)
> +		return ret;
> +
> +	dev->ports = devm_kzalloc(dev->dev,
> +				  sizeof(*dev->ports) * dev->ds->num_ports,
> +				  GFP_KERNEL);

devm_kcalloc?

> +	if (!dev->ports)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < dev->ds->num_ports; i++) {
> +		ret = xrs700x_alloc_port_mib(dev, i);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = dsa_register_switch(dev->ds);
> +
> +	return ret;

return dsa_register_switch

> +}
> +EXPORT_SYMBOL(xrs700x_switch_register);
> +
> +void xrs700x_switch_remove(struct xrs700x *dev)
> +{
> +	cancel_delayed_work_sync(&dev->mib_work);

Is it not enough that this is called from xrs700x_teardown too, which is
in the call path of dsa_unregister_switch below?

> +
> +	dsa_unregister_switch(dev->ds);
> +}
> +EXPORT_SYMBOL(xrs700x_switch_remove);
> diff --git a/drivers/net/dsa/xrs700x/xrs700x_mdio.c b/drivers/net/dsa/xrs700x/xrs700x_mdio.c
> new file mode 100644
> index 000000000000..4fa6cc8f871c
> --- /dev/null
> +++ b/drivers/net/dsa/xrs700x/xrs700x_mdio.c
> +static int xrs700x_mdio_reg_read(void *context, unsigned int reg,
> +				 unsigned int *val)
> +{
> +	struct mdio_device *mdiodev = context;
> +	struct device *dev = &mdiodev->dev;
> +	u16 uval;
> +	int ret;
> +
> +	uval = (u16)FIELD_GET(GENMASK(31, 16), reg);
> +
> +	ret = mdiobus_write(mdiodev->bus, mdiodev->addr, XRS_MDIO_IBA1, uval);
> +	if (ret < 0) {
> +		dev_err(dev, "xrs mdiobus_write returned %d\n", ret);
> +		return ret;
> +	}
> +
> +	uval = (u16)((reg & GENMASK(15, 1)) | XRS_IB_READ);

What happened to bit 0 of "reg"?

> +
> +	ret = mdiobus_write(mdiodev->bus, mdiodev->addr, XRS_MDIO_IBA0, uval);
> +	if (ret < 0) {
> +		dev_err(dev, "xrs mdiobus_write returned %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = mdiobus_read(mdiodev->bus, mdiodev->addr, XRS_MDIO_IBD);
> +	if (ret < 0) {
> +		dev_err(dev, "xrs mdiobus_read returned %d\n", ret);
> +		return ret;
> +	}
> +
> +	*val = (unsigned int)ret;
> +
> +	return 0;
> +}

> +static int xrs700x_mdio_probe(struct mdio_device *mdiodev)
> +{
> +	struct xrs700x *dev;

May boil down to preference too, but I don't believe "dev" is a happy
name to give to a driver private data structure.