Re: [RFC v6 net-next 9/9] net: dsa: ocelot: add external ocelot switch control

[Colin Foster]

Hi Vladimir,

My apologies for the delay. As I mentioned in another thread, I went
through the "MFD" updates before getting to these. A couple questions
that might be helpful before I go to the next RFC.

On Mon, Jan 31, 2022 at 06:50:44PM +0000, Vladimir Oltean wrote:
> On Sat, Jan 29, 2022 at 02:02:21PM -0800, Colin Foster wrote:
> > Add control of an external VSC7512 chip by way of the ocelot-mfd interface.
> > 
> > Currently the four copper phy ports are fully functional. Communication to
> > external phys is also functional, but the SGMII / QSGMII interfaces are
> > currently non-functional.
> > 
> > Signed-off-by: Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/mfd/ocelot-core.c           |   4 +
> >  drivers/net/dsa/ocelot/Kconfig      |  14 +
> >  drivers/net/dsa/ocelot/Makefile     |   5 +
> >  drivers/net/dsa/ocelot/ocelot_ext.c | 681 ++++++++++++++++++++++++++++
> >  include/soc/mscc/ocelot.h           |   2 +
> >  5 files changed, 706 insertions(+)
> >  create mode 100644 drivers/net/dsa/ocelot/ocelot_ext.c
> > 
> > diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> > index 590489481b8c..17a77d618e92 100644
> > --- a/drivers/mfd/ocelot-core.c
> > +++ b/drivers/mfd/ocelot-core.c
> > @@ -122,6 +122,10 @@ static const struct mfd_cell vsc7512_devs[] = {
> >  		.num_resources = ARRAY_SIZE(vsc7512_miim1_resources),
> >  		.resources = vsc7512_miim1_resources,
> >  	},
> > +	{
> > +		.name = "ocelot-ext-switch",
> > +		.of_compatible = "mscc,vsc7512-ext-switch",
> > +	},
> >  };
> >  
> >  int ocelot_core_init(struct ocelot_core *core)
> > diff --git a/drivers/net/dsa/ocelot/Kconfig b/drivers/net/dsa/ocelot/Kconfig
> > index 220b0b027b55..f40b2c7171ad 100644
> > --- a/drivers/net/dsa/ocelot/Kconfig
> > +++ b/drivers/net/dsa/ocelot/Kconfig
> > @@ -1,4 +1,18 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > +config NET_DSA_MSCC_OCELOT_EXT
> > +	tristate "Ocelot External Ethernet switch support"
> > +	depends on NET_DSA && SPI
> > +	depends on NET_VENDOR_MICROSEMI
> > +	select MDIO_MSCC_MIIM
> > +	select MFD_OCELOT_CORE
> > +	select MSCC_OCELOT_SWITCH_LIB
> > +	select NET_DSA_TAG_OCELOT_8021Q
> > +	select NET_DSA_TAG_OCELOT
> > +	help
> > +	  This driver supports the VSC7511, VSC7512, VSC7513 and VSC7514 chips
> > +	  when controlled through SPI. It can be used with the Microsemi dev
> > +	  boards and an external CPU or custom hardware.
> > +
> >  config NET_DSA_MSCC_FELIX
> >  	tristate "Ocelot / Felix Ethernet switch support"
> >  	depends on NET_DSA && PCI
> > diff --git a/drivers/net/dsa/ocelot/Makefile b/drivers/net/dsa/ocelot/Makefile
> > index f6dd131e7491..d7f3f5a4461c 100644
> > --- a/drivers/net/dsa/ocelot/Makefile
> > +++ b/drivers/net/dsa/ocelot/Makefile
> > @@ -1,11 +1,16 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >  obj-$(CONFIG_NET_DSA_MSCC_FELIX) += mscc_felix.o
> > +obj-$(CONFIG_NET_DSA_MSCC_OCELOT_EXT) += mscc_ocelot_ext.o
> >  obj-$(CONFIG_NET_DSA_MSCC_SEVILLE) += mscc_seville.o
> >  
> >  mscc_felix-objs := \
> >  	felix.o \
> >  	felix_vsc9959.o
> >  
> > +mscc_ocelot_ext-objs := \
> > +	felix.o \
> > +	ocelot_ext.o
> > +
> >  mscc_seville-objs := \
> >  	felix.o \
> >  	seville_vsc9953.o
> > diff --git a/drivers/net/dsa/ocelot/ocelot_ext.c b/drivers/net/dsa/ocelot/ocelot_ext.c
> > new file mode 100644
> > index 000000000000..6fdff016673e
> > --- /dev/null
> > +++ b/drivers/net/dsa/ocelot/ocelot_ext.c
> 
> How about ocelot_vsc7512.c for a name?

I'm not crazy about "ocelot_ext" either... but I intend for this to
support VSC7511, 7512, 7513, and 7514. I'm using 7512 as my starting
point, but 7511 will be in quick succession, so I don't think
ocelot_vsc7512 is appropriate.

I'll update everything that is 7512-specific to be appropriately named.
Addresses, features, etc. As you suggest below, there's some function
names that are still around with the vsc7512 name that I'm changing to
the more generic "ocelot_ext" version.

[ ... ]
> > +static struct ocelot_ext_data *felix_to_ocelot_ext(struct felix *felix)
> > +{
> > +	return container_of(felix, struct ocelot_ext_data, felix);
> > +}
> > +
> > +static struct ocelot_ext_data *ocelot_to_ocelot_ext(struct ocelot *ocelot)
> > +{
> > +	struct felix *felix = ocelot_to_felix(ocelot);
> > +
> > +	return felix_to_ocelot_ext(felix);
> > +}
> 
> I wouldn't mind a "ds_to_felix()" helper, but as mentioned, it would be
> good if you could use struct felix instead of introducing yet one more
> container.
> 

Currently the ocelot_ext struct is unused, and will be removed from v7,
along with these container conversions. I'll keep this in mind if I end
up needing to expand things in the future.

When these were written it was clear that "Felix" had no business
dragging around info about "ocelot_spi," so these conversions seemed
necessary. Now that SPI has been completely removed from this DSA
section, things are a lot cleaner.

> > +
> > +static void ocelot_ext_reset_phys(struct ocelot *ocelot)
> > +{
> > +	ocelot_write(ocelot, 0, GCB_PHY_PHY_CFG);
> > +	ocelot_write(ocelot, 0x1ff, GCB_PHY_PHY_CFG);
> > +	mdelay(500);
> > +}
> > +
> > +static int ocelot_ext_reset(struct ocelot *ocelot)
> > +{
> > +	struct felix *felix = ocelot_to_felix(ocelot);
> > +	struct device *dev = ocelot->dev;
> > +	struct device_node *mdio_node;
> > +	int retries = 100;
> > +	int err, val;
> > +
> > +	ocelot_ext_reset_phys(ocelot);
> > +
> > +	mdio_node = of_get_child_by_name(dev->of_node, "mdio");
> 
>  * Return: A node pointer if found, with refcount incremented, use
>  * of_node_put() on it when done.
> 
> There's no "of_node_put()" below.
> 
> > +	if (!mdio_node)
> > +		dev_info(ocelot->dev,
> > +			 "mdio children not found in device tree\n");
> > +
> > +	err = of_mdiobus_register(felix->imdio, mdio_node);
> > +	if (err) {
> > +		dev_err(ocelot->dev, "error registering MDIO bus\n");
> > +		return err;
> > +	}
> > +
> > +	felix->ds->slave_mii_bus = felix->imdio;
> 
> A bit surprised to see MDIO bus registration in ocelot_ops :: reset and
> not in felix_info :: mdio_bus_alloc.

These are both good catches. Thanks! This one in particular was a relic
of the initial spi_device design - no communication could have been
performed at all until after the bus was getting initailized... which
was in reset at the time.

Now it is in the MFD core initialization.

This brings up a question that I think you were getting at when MFD was
first discussed for this driver:

Should Felix know anything about the chip's internal MDIO bus? Or should
the internal bus be a separate entry in the MFD?

Currently my DT is structured as:

&spi0 {
        ocelot-chip@0 {
                compatible = "mscc,vsc7512_mfd_spi";
                ethernet-switch@0 {
                        compatible = "mscc,vsc7512-ext-switch";
                        ports {
                        };

                        /* Internal MDIO port here */
                        mdio {
                        };
                };
                /* External MDIO port here */
                mdio1: mdio1 {
                        compatible = "mscc,ocelot-miim";
                };
                /* Additional peripherals here - pinctrl, sgpio, hsio... */
                gpio: pinctrl@0 {
                        compatible = "mscc,ocelot-pinctrl"
                };
                ...
        };
};


Should it instead be:

&spi0 {
        ocelot-chip@0 {
                compatible = "mscc,vsc7512_mfd_spi";
                ethernet-switch@0 {
                        compatible = "mscc,vsc7512-ext-switch";
                        ports {
                        };
                };
                /* Internal MDIO port here */
                mdio0: mdio0 {
                        compatible = "mscc,ocelot-miim"
                };
                /* External MDIO port here */
                mdio1: mdio1 {
                        compatible = "mscc,ocelot-miim";
                };
                /* Additional peripherals here - pinctrl, sgpio, hsio... */
                gpio: pinctrl@0 {
                        compatible = "mscc,ocelot-pinctrl"
                };
                ...
        };
};

That way I could get rid of mdio_bus_alloc entirely. (I just tried it
and it didn't "just work" but I'll do a little debugging)

The more I think about it the more I think this is the correct path to
go down.

[ ... ]
> > +		return err;
> > +
> > +	err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA], 1);
> > +	if (err)
> > +		return err;
> > +
> > +	do {
> > +		msleep(1);
> > +		regmap_field_read(ocelot->regfields[SYS_RESET_CFG_MEM_INIT],
> > +				  &val);
> > +	} while (val && --retries);
> > +
> > +	if (!retries)
> > +		return -ETIMEDOUT;
> > +
> > +	err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_CORE_ENA], 1);
> > +
> > +	return err;
> 
> "err = ...; return err" can be turned into "return ..." if it weren't
> for error handling. But you need to handle errors.

With this error handling during a reset... these errors get handled in
the main ocelot switch library by way of ocelot->ops->reset().

I can add additional dev_err messages on all these calls if that would
be useful.

[ ... ]
> > +static void vsc7512_mdio_bus_free(struct ocelot *ocelot)
> > +{
> > +	struct felix *felix = ocelot_to_felix(ocelot);
> > +
> > +	if (felix->imdio)
> 
> I don't think the conditional is warranted here? Did you notice a call
> path where you were called while felix->imdio was NULL?
> 

You're right. It was probably necessary for me to get off the ground,
but not anymore. Removed.

[ ... ]
> > +static int ocelot_ext_probe(struct platform_device *pdev)
> > +{
> > +	struct ocelot_ext_data *ocelot_ext;
> > +	struct dsa_switch *ds;
> > +	struct ocelot *ocelot;
> > +	struct felix *felix;
> > +	struct device *dev;
> > +	int err;
> > +
> > +	dev = &pdev->dev;
> > +
> > +	ocelot_ext = devm_kzalloc(dev, sizeof(struct ocelot_ext_data),
> > +				  GFP_KERNEL);
> > +
> > +	if (!ocelot_ext)
> 
> Try to omit blank lines between an assignment and the proceeding sanity
> checks. Also, try to stick to either using devres everywhere, or nowhere,
> within the same function at least.

I switched both calls to not use devres and free both of these in remove
now. However... (comments below)

> 
> > +		return -ENOMEM;
> > +
> > +	dev_set_drvdata(dev, ocelot_ext);
> > +
> > +	ocelot_ext->port_modes = vsc7512_port_modes;
> > +	felix = &ocelot_ext->felix;
> > +
> > +	ocelot = &felix->ocelot;
> > +	ocelot->dev = dev;
> > +
> > +	ocelot->num_flooding_pgids = 1;
> > +
> > +	felix->info = &ocelot_ext_info;
> > +
> > +	ds = kzalloc(sizeof(*ds), GFP_KERNEL);
> > +	if (!ds) {
> > +		err = -ENOMEM;
> > +		dev_err(dev, "Failed to allocate DSA switch\n");
> > +		return err;
> > +	}
> > +
> > +	ds->dev = dev;
> > +	ds->num_ports = felix->info->num_ports;
> > +	ds->num_tx_queues = felix->info->num_tx_queues;
> > +
> > +	ds->ops = &felix_switch_ops;
> > +	ds->priv = ocelot;
> > +	felix->ds = ds;
> > +	felix->tag_proto = DSA_TAG_PROTO_OCELOT;
> > +
> > +	err = dsa_register_switch(ds);
> > +
> > +	if (err) {
> > +		dev_err(dev, "Failed to register DSA switch: %d\n", err);
> > +		goto err_register_ds;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_register_ds:
> > +	kfree(ds);
> > +	return err;
> > +}
> > +
> > +static int ocelot_ext_remove(struct platform_device *pdev)
> > +{
> > +	struct ocelot_ext_data *ocelot_ext;
> > +	struct felix *felix;
> > +
> > +	ocelot_ext = dev_get_drvdata(&pdev->dev);
> > +	felix = &ocelot_ext->felix;
> > +
> > +	dsa_unregister_switch(felix->ds);
> > +
> > +	kfree(felix->ds);
> > +
> > +	devm_kfree(&pdev->dev, ocelot_ext);
> 
> What is the point of devm_kfree?
> 
> > +
> > +	return 0;
> > +}
> > +
> > +const struct of_device_id ocelot_ext_switch_of_match[] = {
> > +	{ .compatible = "mscc,vsc7512-ext-switch" },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, ocelot_ext_switch_of_match);
> > +
> > +static struct platform_driver ocelot_ext_switch_driver = {
> > +	.driver = {
> > +		.name = "ocelot-ext-switch",
> > +		.of_match_table = of_match_ptr(ocelot_ext_switch_of_match),
> > +	},
> > +	.probe = ocelot_ext_probe,
> > +	.remove = ocelot_ext_remove,
> 
> Please blindly follow the pattern of every other DSA driver, with a
> ->remove and ->shutdown method that run either one, or the other, by
> checking whether dev_get_drvdata() has been set to NULL by the other one
> or not. And call dsa_switch_shutdown() from ocelot_ext_shutdown() (or
> vsc7512_shutdown, or whatever you decide to call it).

... I assume there's no worry that kfree gets called in each driver's
remove routine but not in their shutdown? I'll read through commit
0650bf52b31f (net: dsa: be compatible with masters which unregister on shutdown)
to get a more thorough understanding of what's going on... but will
blindly follow for now. :-)

> 
> > +};
> > +module_platform_driver(ocelot_ext_switch_driver);
> > +
> > +MODULE_DESCRIPTION("External Ocelot Switch driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> > index 8b8ebede5a01..62cd61d4142e 100644
> > --- a/include/soc/mscc/ocelot.h
> > +++ b/include/soc/mscc/ocelot.h
> > @@ -399,6 +399,8 @@ enum ocelot_reg {
> >  	GCB_MIIM_MII_STATUS,
> >  	GCB_MIIM_MII_CMD,
> >  	GCB_MIIM_MII_DATA,
> > +	GCB_PHY_PHY_CFG,
> > +	GCB_PHY_PHY_STAT,
> >  	DEV_CLOCK_CFG = DEV_GMII << TARGET_OFFSET,
> >  	DEV_PORT_MISC,
> >  	DEV_EVENTS,
> > -- 
> > 2.25.1
> >