On Thu, Jan 14, 2021 at 11:28 AM Florian Fainelli <f.fainelli@xxxxxxxxx> wrote:
>
> 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.
Okay, changed to a loop using eth_stp_addr. I'll retest STP to make
sure I didn't break it (this would be my luck towards the end of a
patch series).
>
> [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.
Sounds good.
>
> > +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.
In case of an error probe will end up returning an error and the
memory will be free'd since it was allocated with a devm_ function,
won't it?
>
> [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.
Okay, I moved it.
>
> [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?
Sure.
> --
> Florian
