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

[George McCollister <george.mccollister@gmail.com>]

On Wed, Jan 13, 2021 at 7:57 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> What PHY interface types does the switch support as of this patch?
> No RGMII delay configuration needed?
>

Port 0: RMII
Port 1-3: RGMII

For RGMII the documentation states:
"PCB is required to add 1.5 ns to 2.0 ns more delay to the clock line
than the other lines, unless the other end (PHY) has configurable RX
clock delay."

> > +
> > +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.

Okay, done.

>
> > +             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.

Previously Andrew said:
"Either you need to verify the compatible from day one so it is not
wrong, or you just use a single compatible "arrow,xrs700x", which
cannot be wrong."

I did it the first way he suggested, if you would have replied at that
time to use a single that's the way I would have done it that way.

If you two can agree I should change it to a single string I'd be
happy to do so. In my case I need 3 RGMII and only one of the package
types will fit on the board so there's no risk of changing to one of
the existing parts. Perhaps this could be an issue if a new part is
added in the future or on someone else's design.

>
> > +
> > +     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.

The second line uses p so that won't work. I'll change the function to
use devm_kcalloc like you recommended below and just get rid of
mib_size.

>
> > +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?

Ok, done.


>
> > +     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?

yeah, looks like it. I'll remove this.

>
> > +
> > +     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"?

From the datasheet:
"Bits 15-1 of the address on the internal bus to where data is written
or from where data is read. Address bit 0 is always 0 (because of 16
bit registers)."

reg_stride is set to 2.
"The register address stride. Valid register addresses are a multiple
of this value."

>
> > +
> > +     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.

There are other drivers in the subsystem that do this. If there was a
consistent pattern followed in the subsystem I would have followed it.
Trust me I was a bit frustrated with home much time I spent going
through multiple drivers trying to determine the best practices for
organization, naming, etc.
If it's a big let me know and I'll change it.

Thanks,
George