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

[Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>]

Hi Vladimir,

On Mon, Mar 07, 2022 at 09:51:38PM +0000, Vladimir Oltean wrote:
> 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.

I see. And there was a comment a while back... I believe it was
Alexandre suggested there was some of consideration in the design to
support the non-OF-based cases. I hope I'm getting a better idea of the
big picture... one piece at a time.

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

Yes, you suggested this early on. Thank you for guiding me down the
right path.

> 
> 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(@.*)?".

Safe travels!

I was really surprised about the two MDIO buses as well. My coworker
pointed this out to me right before I decided to start looking into the
external phys and probably saved me a week of datasheet shuffling / scope
probing. Especially since the MDIO bus 2 addresses are pin-strapped to 
start at 4, seemingly to not overlap the internal MDIO addresses 0-3.

And the two MDIO buses also exist in
arch/mips/boot/dts/mscc/ocelot.dtsi, so I know I'm not crazy.

I'll update the node names in my tree per your suggestion. I figured
there'd be no desire for me sharing a .dtsi for my boot-pin-modified dev
board configuration. Maybe I'm wrong, and sharing the relevant portions
in cover letters is not the right thing to do.

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

Thanks for explaining this. Understood.

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

Interesting concept. Makes sense though. Thanks again for explaining!

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