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

[Vladimir Oltean <vladimir.oltean@xxxxxxx>]

On Sat, Mar 05, 2022 at 04:28:49PM -0800, Colin Foster wrote:
> 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.

As I've mentioned in the past, on NXP switches (felix/seville), there
was a different justification. There, the internal MDIO bus is used to
access the SGMII PCS, not any internal PHY as in the ocelot-ext case.
As opposed to the 'phy-handle' that describes the relationship between a
MAC and its (internal) PHY, no such equivalent 'pcs-handle' property
exists in a standardized form. So I wanted to avoid a dependency on OF
where the drivers would not learn any actual information from it.

It is also possible to have a non-OF based connection to the internal
PHY, but that has some limitations, because DSA has a lot of legacy in
this area. 'Non OF-based' means that there is a port which lacks both
'phy-handle' and 'fixed-link'. We have said that a user port with such
an OF node should be interpreted as having an internal PHY located on
the ds->slave_mii_bus at a PHY address equal to the port index.
Whereas the same conditions (no 'phy-handle', no 'fixed-link') on a CPU
port mean that the port is a fixed-link that operates at the largest
supported link speed.

Since you have a PHY on the CPU port, I'd tend to avoid any ambiguity
and explicitly specify the 'phy-handle', 'fixed-link' properties in the
device tree.

What I'm not completely sure about is whether you really have 2 MDIO
buses. I don't have a VSC7512, and I haven't checked the datasheet
(traveling right now) but this would be surprising to me.
Anyway, if you do, then at least try to match the $nodename pattern from
Documentation/devicetree/bindings/net/mdio.yaml. I don't think "mdio0"
matches "^mdio(@.*)?".

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

Please interpret this in context. Your ocelot_ext_reset() function calls
of_mdiobus_register(), then does other work which may fail, then returns
that error code while leaving the MDIO bus dangling. When I said "you
need to handle errors" I meant "you need to unwind whatever work is done
in the function in the case of an error". If you are going to remove the
of_mdiobus_register(), there is probably not much left.

> [ ... ]
> > > +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. :-)

The remove method is called when you unbind the driver from the
device. The shutdown method is called when you reboot. The latter can be
leaky w.r.t. memory allocation.

My request here was to provide a shutdown method implementation, and
hook it in the same way as other DSA drivers do.

> > 
> > > +};
> > > +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
> > >