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@xxxxxxxxx>
> ---
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.
