On 1/13/21 6:59 AM, 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>
> ---
[snip]
This looks ready to me, just a few nits and suggestions.
> +/* Add an inbound policy filter which matches the BPDU destination MAC
> + * and forwards to the CPU port. Leave the policy disabled, it will be
> + * enabled as needed.
> + */
> +static int xrs700x_port_add_bpdu_ipf(struct dsa_switch *ds, int port)
> +{
> + struct xrs700x *priv = ds->priv;
> + unsigned int val = 0;
> + int i = 0;
> + int ret;
> +
> + /* Compare all 48 bits of the destination MAC address. */
> + ret = regmap_write(priv->regmap, XRS_ETH_ADDR_CFG(port, 0), 48 << 2);
> + if (ret)
> + return ret;
> +
> + /* match BPDU destination 01:80:c2:00:00:00 */
> + ret = regmap_write(priv->regmap, XRS_ETH_ADDR_0(port, 0), 0x8001);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, XRS_ETH_ADDR_1(port, 0), 0xc2);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, XRS_ETH_ADDR_2(port, 0), 0x0);
> + if (ret)
> + return ret;
Not that this is likely to change, but you could write this as a for
loop and use eth_stp_addr from include/linux/etherdevice.h.
[snip]
> +static int xrs700x_port_setup(struct dsa_switch *ds, int port)
> +{
> + bool cpu_port = dsa_is_cpu_port(ds, port);
> + struct xrs700x *priv = ds->priv;
> + unsigned int val = 0;
> + int ret, i;
> +
> + xrs700x_port_stp_state_set(ds, port, BR_STATE_DISABLED);
It looks like we should be standardizing at some point on having switch
drivers do just the global configuration in the ->setup() callback and
have the core call the ->port_disable() for each port except the CPU/DSA
ports, and finally let the actual port configuration bet done in
->port_enable(). What you have is fine for now and easy to change in the
future.
> +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);
> + 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;
Nothing frees up the successfully allocated p->mib_data[] in case of
errors so you would be leaking here.
[snip]
> +
> + /* Main DSA driver may not be started yet. */
> + if (ret)
> + return ret;
> +
> + i2c_set_clientdata(i2c, dev);
I would be tempted to move this before "publishing" the device, probably
does not harm though, likewise for the MDIO stub.
[snip]
> +/* Switch Configuration Registers - VLAN */
> +#define XRS_VLAN(v) (XRS_SWITCH_VLAN_BASE + 0x2 * (v))
> +
> +#define MAX_VLAN 4095
Can you use VLAN_N_VID - 1 here from include/linux/if_vlan.h?
--
Florian
