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:

quoted

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:

quoted

On Sat, Jan 29, 2022 at 02:02:21PM -0800, Colin Foster wrote:

quoted

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@in-advantage.com>
---
 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.

[ ... ]

quoted

quoted

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

quoted

quoted

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

quoted

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

quoted

[ ... ]

quoted

quoted

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

quoted

[ ... ]

quoted

quoted

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

[ ... ]

quoted

quoted

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

quoted

quoted

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

quoted

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

quoted

quoted

quoted

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