This patch series is a continuation to add support for the VSC7512:
https://patchwork.kernel.org/project/netdevbpf/list/?series=674168&state=*
That series added the framework and initial functionality for the
VSC7512 chip. Several of these patches grew during the initial
development of the framework, which is why v1 will include changelogs.
It was during v9 of that original MFD patch set that these were dropped.
With that out of the way, the VSC7512 is mainly a subset of the VSC7514
chip. The 7512 lacks an internal MIPS processor, but otherwise many of
the register definitions are identical. That is why several of these
patches are simply to expose common resources from
drivers/net/ethernet/mscc/*.
This patch only adds support for the first four ports (swp0-swp3). The
remaining ports require more significant changes to the felix driver,
and will be handled in the future.
Lastly, this patch set currently won't apply cleanly to net-next, as it
requires a sync with the MFD tree. This is being sent as an RFC, and
additional changes (e.g. documentation) will be required, so I expect
this won't be ready to become a PATCH until the next version (v6.2)
v1 (from RFC v8 suggested above):
* Utilize the MFD framework for creating regmaps, as well as
dev_get_regmap() (patches 7 and 8 of this series)
Colin Foster (8):
net: mscc: ocelot: expose ocelot wm functions
net: mscc: ocelot: expose regfield definition to be used by other
drivers
net: mscc: ocelot: expose stats layout definition to be used by other
drivers
net: mscc: ocelot: expose vcap_props structure
net: dsa: felix: add configurable device quirks
net: dsa: felix: populate mac_capabilities for all ports
mfd: ocelot: add regmaps for ocelot_ext
net: dsa: ocelot: add external ocelot switch control
drivers/mfd/ocelot-core.c | 91 +++++++-
drivers/net/dsa/ocelot/Kconfig | 14 ++
drivers/net/dsa/ocelot/Makefile | 5 +
drivers/net/dsa/ocelot/felix.c | 10 +-
drivers/net/dsa/ocelot/felix.h | 1 +
drivers/net/dsa/ocelot/felix_vsc9959.c | 1 +
drivers/net/dsa/ocelot/ocelot_ext.c | 254 +++++++++++++++++++++
drivers/net/dsa/ocelot/seville_vsc9953.c | 1 +
drivers/net/ethernet/mscc/ocelot_devlink.c | 31 +++
drivers/net/ethernet/mscc/ocelot_vsc7514.c | 137 +----------
drivers/net/ethernet/mscc/vsc7514_regs.c | 108 +++++++++
include/linux/mfd/ocelot.h | 5 +
include/soc/mscc/ocelot.h | 7 +
include/soc/mscc/vsc7514_regs.h | 6 +
14 files changed, 529 insertions(+), 142 deletions(-)
create mode 100644 drivers/net/dsa/ocelot/ocelot_ext.c
--
2.25.1
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 <[email protected]>
---
v1 from previous RFC:
* Remove unnecessary byteorder and kconfig header includes.
* Create OCELOT_EXT_PORT_MODE_SERDES macro to match vsc9959.
* Utilize readx_poll_timeout for SYS_RESET_CFG_MEM_INIT.
* *_io_res struct arrays have been moved to the MFD files.
* Changes to utilize phylink_generic_validate() have been squashed.
* dev_err_probe() is used in the probe function.
* Make ocelot_ext_switch_of_match static.
* Relocate ocelot_ext_ops structure to be next to vsc7512_info, to
match what was done in other felix drivers.
* Utilize dev_get_regmap() instead of the obsolete
ocelot_init_regmap_from_resource() routine.
---
drivers/mfd/ocelot-core.c | 3 +
drivers/net/dsa/ocelot/Kconfig | 14 ++
drivers/net/dsa/ocelot/Makefile | 5 +
drivers/net/dsa/ocelot/ocelot_ext.c | 254 ++++++++++++++++++++++++++++
include/soc/mscc/ocelot.h | 2 +
5 files changed, 278 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 aa7fa21b354c..b7b9f6855f74 100644
--- a/drivers/mfd/ocelot-core.c
+++ b/drivers/mfd/ocelot-core.c
@@ -188,6 +188,9 @@ static const struct mfd_cell vsc7512_devs[] = {
.use_of_reg = true,
.num_resources = ARRAY_SIZE(vsc7512_miim1_resources),
.resources = vsc7512_miim1_resources,
+ }, {
+ .name = "ocelot-ext-switch",
+ .of_compatible = "mscc,vsc7512-ext-switch",
},
};
diff --git a/drivers/net/dsa/ocelot/Kconfig b/drivers/net/dsa/ocelot/Kconfig
index 08db9cf76818..d8b224f8dc97 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..c821cc963787
--- /dev/null
+++ b/drivers/net/dsa/ocelot/ocelot_ext.c
@@ -0,0 +1,254 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/*
+ * Copyright 2021-2022 Innovative Advantage Inc.
+ */
+
+#include <linux/iopoll.h>
+#include <linux/mfd/ocelot.h>
+#include <linux/phylink.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <soc/mscc/ocelot_ana.h>
+#include <soc/mscc/ocelot_dev.h>
+#include <soc/mscc/ocelot_qsys.h>
+#include <soc/mscc/ocelot_vcap.h>
+#include <soc/mscc/ocelot_ptp.h>
+#include <soc/mscc/ocelot_sys.h>
+#include <soc/mscc/ocelot.h>
+#include <soc/mscc/vsc7514_regs.h>
+#include "felix.h"
+
+#define VSC7512_NUM_PORTS 11
+
+#define OCELOT_EXT_MEM_INIT_SLEEP_US 1000
+#define OCELOT_EXT_MEM_INIT_TIMEOUT_US 100000
+
+#define OCELOT_EXT_PORT_MODE_SERDES (OCELOT_PORT_MODE_SGMII | \
+ OCELOT_PORT_MODE_QSGMII)
+
+static const u32 vsc7512_port_modes[VSC7512_NUM_PORTS] = {
+ OCELOT_PORT_MODE_INTERNAL,
+ OCELOT_PORT_MODE_INTERNAL,
+ OCELOT_PORT_MODE_INTERNAL,
+ OCELOT_PORT_MODE_INTERNAL,
+ OCELOT_EXT_PORT_MODE_SERDES,
+ OCELOT_EXT_PORT_MODE_SERDES,
+ OCELOT_EXT_PORT_MODE_SERDES,
+ OCELOT_EXT_PORT_MODE_SERDES,
+ OCELOT_EXT_PORT_MODE_SERDES,
+ OCELOT_EXT_PORT_MODE_SERDES,
+ OCELOT_PORT_MODE_SGMII,
+ OCELOT_EXT_PORT_MODE_SERDES,
+};
+
+static const u32 vsc7512_gcb_regmap[] = {
+ REG(GCB_SOFT_RST, 0x0008),
+ REG(GCB_MIIM_MII_STATUS, 0x009c),
+ REG(GCB_PHY_PHY_CFG, 0x00f0),
+ REG(GCB_PHY_PHY_STAT, 0x00f4),
+};
+
+static const u32 *vsc7512_regmap[TARGET_MAX] = {
+ [ANA] = vsc7514_ana_regmap,
+ [QS] = vsc7514_qs_regmap,
+ [QSYS] = vsc7514_qsys_regmap,
+ [REW] = vsc7514_rew_regmap,
+ [SYS] = vsc7514_sys_regmap,
+ [S0] = vsc7514_vcap_regmap,
+ [S1] = vsc7514_vcap_regmap,
+ [S2] = vsc7514_vcap_regmap,
+ [PTP] = vsc7514_ptp_regmap,
+ [GCB] = vsc7512_gcb_regmap,
+ [DEV_GMII] = vsc7514_dev_gmii_regmap,
+};
+
+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_mem_init_status(struct ocelot *ocelot)
+{
+ int val, err;
+
+ err = regmap_field_read(ocelot->regfields[SYS_RESET_CFG_MEM_INIT], &val);
+
+ return err ?: val;
+}
+
+static int ocelot_ext_reset(struct ocelot *ocelot)
+{
+ int err, val;
+
+ ocelot_ext_reset_phys(ocelot);
+
+ /* Initialize chip memories */
+ err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA], 1);
+ if (err)
+ return err;
+
+ err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_INIT], 1);
+ if (err)
+ return err;
+
+ /* MEM_INIT is a self-clearing bit. Wait for it to be clear (should be
+ * 100us) before enabling the switch core
+ */
+ err = readx_poll_timeout(ocelot_ext_mem_init_status, ocelot, val, !val,
+ OCELOT_EXT_MEM_INIT_SLEEP_US,
+ OCELOT_EXT_MEM_INIT_TIMEOUT_US);
+
+ if (IS_ERR_VALUE(err))
+ return err;
+
+ return regmap_field_write(ocelot->regfields[SYS_RESET_CFG_CORE_ENA], 1);
+}
+
+static void ocelot_ext_phylink_validate(struct ocelot *ocelot, int port,
+ unsigned long *supported,
+ struct phylink_link_state *state)
+{
+ struct felix *felix = ocelot_to_felix(ocelot);
+ struct dsa_switch *ds = felix->ds;
+ struct phylink_config *pl_config;
+ struct dsa_port *dp;
+
+ dp = dsa_to_port(ds, port);
+ pl_config = &dp->pl_config;
+
+ phylink_generic_validate(pl_config, supported, state);
+}
+
+static struct regmap *ocelot_ext_regmap_init(struct ocelot *ocelot,
+ struct resource *res)
+{
+ return dev_get_regmap(ocelot->dev->parent, res->name);
+}
+
+static const struct ocelot_ops ocelot_ext_ops = {
+ .reset = ocelot_ext_reset,
+ .wm_enc = ocelot_wm_enc,
+ .wm_dec = ocelot_wm_dec,
+ .wm_stat = ocelot_wm_stat,
+ .port_to_netdev = felix_port_to_netdev,
+ .netdev_to_port = felix_netdev_to_port,
+};
+
+static const struct felix_info vsc7512_info = {
+ .target_io_res = vsc7512_target_io_res,
+ .port_io_res = vsc7512_port_io_res,
+ .regfields = vsc7514_regfields,
+ .map = vsc7512_regmap,
+ .ops = &ocelot_ext_ops,
+ .stats_layout = vsc7514_stats_layout,
+ .vcap = vsc7514_vcap_props,
+ .num_mact_rows = 1024,
+ .num_ports = VSC7512_NUM_PORTS,
+ .num_tx_queues = OCELOT_NUM_TC,
+ .phylink_validate = ocelot_ext_phylink_validate,
+ .port_modes = vsc7512_port_modes,
+ .init_regmap = ocelot_ext_regmap_init,
+};
+
+static int ocelot_ext_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct dsa_switch *ds;
+ struct ocelot *ocelot;
+ struct felix *felix;
+ int err;
+
+ felix = kzalloc(sizeof(*felix), GFP_KERNEL);
+ if (!felix)
+ return -ENOMEM;
+
+ dev_set_drvdata(dev, felix);
+
+ ocelot = &felix->ocelot;
+ ocelot->dev = dev;
+
+ ocelot->num_flooding_pgids = 1;
+
+ felix->info = &vsc7512_info;
+
+ ds = kzalloc(sizeof(*ds), GFP_KERNEL);
+ if (!ds) {
+ err = -ENOMEM;
+ dev_err_probe(dev, err, "Failed to allocate DSA switch\n");
+ goto err_free_felix;
+ }
+
+ 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_probe(dev, err, "Failed to register DSA switch\n");
+ goto err_free_ds;
+ }
+
+ return 0;
+
+err_free_ds:
+ kfree(ds);
+err_free_felix:
+ kfree(felix);
+ return err;
+}
+
+static int ocelot_ext_remove(struct platform_device *pdev)
+{
+ struct felix *felix = dev_get_drvdata(&pdev->dev);
+
+ if (!felix)
+ return 0;
+
+ dsa_unregister_switch(felix->ds);
+
+ kfree(felix->ds);
+ kfree(felix);
+
+ dev_set_drvdata(&pdev->dev, NULL);
+
+ return 0;
+}
+
+static void ocelot_ext_shutdown(struct platform_device *pdev)
+{
+ struct felix *felix = dev_get_drvdata(&pdev->dev);
+
+ if (!felix)
+ return;
+
+ dsa_switch_shutdown(felix->ds);
+
+ dev_set_drvdata(&pdev->dev, NULL);
+}
+
+static 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,
+ .shutdown = ocelot_ext_shutdown,
+};
+module_platform_driver(ocelot_ext_switch_driver);
+
+MODULE_DESCRIPTION("External Ocelot Switch driver");
+MODULE_LICENSE("GPL");
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 17dd61f36563..2ed38110a6cc 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -460,6 +460,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
On Sun, 11 Sep 2022, 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 <[email protected]>
> ---
>
> v1 from previous RFC:
> * Remove unnecessary byteorder and kconfig header includes.
> * Create OCELOT_EXT_PORT_MODE_SERDES macro to match vsc9959.
> * Utilize readx_poll_timeout for SYS_RESET_CFG_MEM_INIT.
> * *_io_res struct arrays have been moved to the MFD files.
> * Changes to utilize phylink_generic_validate() have been squashed.
> * dev_err_probe() is used in the probe function.
> * Make ocelot_ext_switch_of_match static.
> * Relocate ocelot_ext_ops structure to be next to vsc7512_info, to
> match what was done in other felix drivers.
> * Utilize dev_get_regmap() instead of the obsolete
> ocelot_init_regmap_from_resource() routine.
>
> ---
> drivers/mfd/ocelot-core.c | 3 +
> drivers/net/dsa/ocelot/Kconfig | 14 ++
> drivers/net/dsa/ocelot/Makefile | 5 +
> drivers/net/dsa/ocelot/ocelot_ext.c | 254 ++++++++++++++++++++++++++++
> include/soc/mscc/ocelot.h | 2 +
> 5 files changed, 278 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 aa7fa21b354c..b7b9f6855f74 100644
> --- a/drivers/mfd/ocelot-core.c
> +++ b/drivers/mfd/ocelot-core.c
> @@ -188,6 +188,9 @@ static const struct mfd_cell vsc7512_devs[] = {
> .use_of_reg = true,
> .num_resources = ARRAY_SIZE(vsc7512_miim1_resources),
> .resources = vsc7512_miim1_resources,
> + }, {
> + .name = "ocelot-ext-switch",
> + .of_compatible = "mscc,vsc7512-ext-switch",
> },
> };
Please separate this out into its own patch.
--
Lee Jones [李琼斯]
On Mon, Sep 12, 2022 at 11:51:14AM +0100, Lee Jones wrote:
> On Sun, 11 Sep 2022, 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 <[email protected]>
> > ---
> >
> > v1 from previous RFC:
> > * Remove unnecessary byteorder and kconfig header includes.
> > * Create OCELOT_EXT_PORT_MODE_SERDES macro to match vsc9959.
> > * Utilize readx_poll_timeout for SYS_RESET_CFG_MEM_INIT.
> > * *_io_res struct arrays have been moved to the MFD files.
> > * Changes to utilize phylink_generic_validate() have been squashed.
> > * dev_err_probe() is used in the probe function.
> > * Make ocelot_ext_switch_of_match static.
> > * Relocate ocelot_ext_ops structure to be next to vsc7512_info, to
> > match what was done in other felix drivers.
> > * Utilize dev_get_regmap() instead of the obsolete
> > ocelot_init_regmap_from_resource() routine.
> >
> > ---
> > drivers/mfd/ocelot-core.c | 3 +
> > drivers/net/dsa/ocelot/Kconfig | 14 ++
> > drivers/net/dsa/ocelot/Makefile | 5 +
> > drivers/net/dsa/ocelot/ocelot_ext.c | 254 ++++++++++++++++++++++++++++
> > include/soc/mscc/ocelot.h | 2 +
> > 5 files changed, 278 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 aa7fa21b354c..b7b9f6855f74 100644
> > --- a/drivers/mfd/ocelot-core.c
> > +++ b/drivers/mfd/ocelot-core.c
> > @@ -188,6 +188,9 @@ static const struct mfd_cell vsc7512_devs[] = {
> > .use_of_reg = true,
> > .num_resources = ARRAY_SIZE(vsc7512_miim1_resources),
> > .resources = vsc7512_miim1_resources,
> > + }, {
> > + .name = "ocelot-ext-switch",
> > + .of_compatible = "mscc,vsc7512-ext-switch",
> > },
> > };
>
> Please separate this out into its own patch.
I'll do that.
>
> --
> Lee Jones [李琼斯]
On Sun, Sep 11, 2022 at 01:02:44PM -0700, Colin Foster wrote:
> index 08db9cf76818..d8b224f8dc97 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.
I would drop the sentence about Microsemi dev boards or custom hardware.
> diff --git a/drivers/net/dsa/ocelot/ocelot_ext.c b/drivers/net/dsa/ocelot/ocelot_ext.c
> new file mode 100644
> index 000000000000..c821cc963787
> --- /dev/null
> +++ b/drivers/net/dsa/ocelot/ocelot_ext.c
> @@ -0,0 +1,254 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright 2021-2022 Innovative Advantage Inc.
> + */
> +
> +#include <linux/iopoll.h>
> +#include <linux/mfd/ocelot.h>
> +#include <linux/phylink.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <soc/mscc/ocelot_ana.h>
> +#include <soc/mscc/ocelot_dev.h>
> +#include <soc/mscc/ocelot_qsys.h>
> +#include <soc/mscc/ocelot_vcap.h>
> +#include <soc/mscc/ocelot_ptp.h>
> +#include <soc/mscc/ocelot_sys.h>
> +#include <soc/mscc/ocelot.h>
> +#include <soc/mscc/vsc7514_regs.h>
> +#include "felix.h"
> +
> +#define VSC7512_NUM_PORTS 11
> +
> +#define OCELOT_EXT_MEM_INIT_SLEEP_US 1000
> +#define OCELOT_EXT_MEM_INIT_TIMEOUT_US 100000
> +
> +#define OCELOT_EXT_PORT_MODE_SERDES (OCELOT_PORT_MODE_SGMII | \
> + OCELOT_PORT_MODE_QSGMII)
There are places where OCELOT_EXT doesn't make too much sense, like here.
The capabilities of the SERDES ports do not change depending on whether
the switch is controlled externally or not. Same for the memory init
delays. Maybe OCELOT_MEM_INIT_*, OCELOT_PORT_MODE_SERDES etc?
There are more places as well below in function names, I'll let you be
the judge if whether ocelot is controlled externally is relevant to what
they do in any way.
> +static int ocelot_ext_reset(struct ocelot *ocelot)
> +{
> + int err, val;
> +
> + ocelot_ext_reset_phys(ocelot);
> +
> + /* Initialize chip memories */
> + err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA], 1);
> + if (err)
> + return err;
> +
> + err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_INIT], 1);
> + if (err)
> + return err;
> +
> + /* MEM_INIT is a self-clearing bit. Wait for it to be clear (should be
> + * 100us) before enabling the switch core
> + */
> + err = readx_poll_timeout(ocelot_ext_mem_init_status, ocelot, val, !val,
> + OCELOT_EXT_MEM_INIT_SLEEP_US,
> + OCELOT_EXT_MEM_INIT_TIMEOUT_US);
> +
I think you can eliminate the newline between the err assignment and
checking for it.
> + if (IS_ERR_VALUE(err))
> + return err;
> +
> + return regmap_field_write(ocelot->regfields[SYS_RESET_CFG_CORE_ENA], 1);
> +}
> +
> +static void ocelot_ext_phylink_validate(struct ocelot *ocelot, int port,
> + unsigned long *supported,
> + struct phylink_link_state *state)
> +{
> + struct felix *felix = ocelot_to_felix(ocelot);
> + struct dsa_switch *ds = felix->ds;
> + struct phylink_config *pl_config;
> + struct dsa_port *dp;
> +
> + dp = dsa_to_port(ds, port);
> + pl_config = &dp->pl_config;
> +
> + phylink_generic_validate(pl_config, supported, state);
You could save 2 lines here (defining *pl_config and assigning it) if
you would just call phylink_generic_validate(&dp->pl_config, ...);
> +}
> +
> +static struct regmap *ocelot_ext_regmap_init(struct ocelot *ocelot,
> + struct resource *res)
> +{
> + return dev_get_regmap(ocelot->dev->parent, res->name);
> +}
I have more fundamental questions about this one, which I've formulated
on your patch 7/8. If nothing changes, at least I'd expect some comments
here explaining where the resources actually come from, and the regmaps.
> +static const struct of_device_id ocelot_ext_switch_of_match[] = {
> + { .compatible = "mscc,vsc7512-ext-switch" },
I think I've raised this before. How about removing "external" from the
compatible string? You can figure out it's external, because it's on a
SPI bus.
> + { },
> +};
> +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,
> + .shutdown = ocelot_ext_shutdown,
> +};
> +module_platform_driver(ocelot_ext_switch_driver);
Hi Vladimir,
On Mon, Sep 12, 2022 at 05:21:10PM +0000, Vladimir Oltean wrote:
> On Sun, Sep 11, 2022 at 01:02:44PM -0700, Colin Foster wrote:
> > index 08db9cf76818..d8b224f8dc97 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.
>
> I would drop the sentence about Microsemi dev boards or custom hardware.
I'll do that. Also I need to add a paragraph (according to checkpatch)
about what the VSC751{1-4} chips actually are.
>
> > diff --git a/drivers/net/dsa/ocelot/ocelot_ext.c b/drivers/net/dsa/ocelot/ocelot_ext.c
> > new file mode 100644
> > index 000000000000..c821cc963787
> > --- /dev/null
> > +++ b/drivers/net/dsa/ocelot/ocelot_ext.c
> > @@ -0,0 +1,254 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright 2021-2022 Innovative Advantage Inc.
> > + */
> > +
> > +#include <linux/iopoll.h>
> > +#include <linux/mfd/ocelot.h>
> > +#include <linux/phylink.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <soc/mscc/ocelot_ana.h>
> > +#include <soc/mscc/ocelot_dev.h>
> > +#include <soc/mscc/ocelot_qsys.h>
> > +#include <soc/mscc/ocelot_vcap.h>
> > +#include <soc/mscc/ocelot_ptp.h>
> > +#include <soc/mscc/ocelot_sys.h>
> > +#include <soc/mscc/ocelot.h>
> > +#include <soc/mscc/vsc7514_regs.h>
> > +#include "felix.h"
> > +
> > +#define VSC7512_NUM_PORTS 11
> > +
> > +#define OCELOT_EXT_MEM_INIT_SLEEP_US 1000
> > +#define OCELOT_EXT_MEM_INIT_TIMEOUT_US 100000
> > +
> > +#define OCELOT_EXT_PORT_MODE_SERDES (OCELOT_PORT_MODE_SGMII | \
> > + OCELOT_PORT_MODE_QSGMII)
>
> There are places where OCELOT_EXT doesn't make too much sense, like here.
> The capabilities of the SERDES ports do not change depending on whether
> the switch is controlled externally or not. Same for the memory init
> delays. Maybe OCELOT_MEM_INIT_*, OCELOT_PORT_MODE_SERDES etc?
>
> There are more places as well below in function names, I'll let you be
> the judge if whether ocelot is controlled externally is relevant to what
> they do in any way.
>
> > +static int ocelot_ext_reset(struct ocelot *ocelot)
> > +{
> > + int err, val;
> > +
> > + ocelot_ext_reset_phys(ocelot);
> > +
> > + /* Initialize chip memories */
> > + err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA], 1);
> > + if (err)
> > + return err;
> > +
> > + err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_INIT], 1);
> > + if (err)
> > + return err;
> > +
> > + /* MEM_INIT is a self-clearing bit. Wait for it to be clear (should be
> > + * 100us) before enabling the switch core
> > + */
> > + err = readx_poll_timeout(ocelot_ext_mem_init_status, ocelot, val, !val,
> > + OCELOT_EXT_MEM_INIT_SLEEP_US,
> > + OCELOT_EXT_MEM_INIT_TIMEOUT_US);
> > +
>
> I think you can eliminate the newline between the err assignment and
> checking for it.
Yes, you've pointed out this habit of mine in the past. I'm sorry you
have to keep reminding me - I'll try to commit this one to memory now.
>
> > + if (IS_ERR_VALUE(err))
> > + return err;
> > +
> > + return regmap_field_write(ocelot->regfields[SYS_RESET_CFG_CORE_ENA], 1);
> > +}
> > +
> > +static void ocelot_ext_phylink_validate(struct ocelot *ocelot, int port,
> > + unsigned long *supported,
> > + struct phylink_link_state *state)
> > +{
> > + struct felix *felix = ocelot_to_felix(ocelot);
> > + struct dsa_switch *ds = felix->ds;
> > + struct phylink_config *pl_config;
> > + struct dsa_port *dp;
> > +
> > + dp = dsa_to_port(ds, port);
> > + pl_config = &dp->pl_config;
> > +
> > + phylink_generic_validate(pl_config, supported, state);
>
> You could save 2 lines here (defining *pl_config and assigning it) if
> you would just call phylink_generic_validate(&dp->pl_config, ...);
Fair enough. Thanks.
>
> > +}
> > +
> > +static struct regmap *ocelot_ext_regmap_init(struct ocelot *ocelot,
> > + struct resource *res)
> > +{
> > + return dev_get_regmap(ocelot->dev->parent, res->name);
> > +}
>
> I have more fundamental questions about this one, which I've formulated
> on your patch 7/8. If nothing changes, at least I'd expect some comments
> here explaining where the resources actually come from, and the regmaps.
Yep. That discussion should address everything anyone might want to
know. Wherever this lands, I'll make sure it is clear to the reader.
>
> > +static const struct of_device_id ocelot_ext_switch_of_match[] = {
> > + { .compatible = "mscc,vsc7512-ext-switch" },
>
> I think I've raised this before. How about removing "external" from the
> compatible string? You can figure out it's external, because it's on a
> SPI bus.
I believe you're right. I'm sorry that slipped through the cracks.
>
> > + { },
> > +};
> > +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,
> > + .shutdown = ocelot_ext_shutdown,
> > +};
> > +module_platform_driver(ocelot_ext_switch_driver);
On Mon, Sep 12, 2022 at 05:21:10PM +0000, Vladimir Oltean wrote:
> On Sun, Sep 11, 2022 at 01:02:44PM -0700, Colin Foster wrote:
> > +
> > +#define OCELOT_EXT_MEM_INIT_SLEEP_US 1000
> > +#define OCELOT_EXT_MEM_INIT_TIMEOUT_US 100000
> > +
> > +#define OCELOT_EXT_PORT_MODE_SERDES (OCELOT_PORT_MODE_SGMII | \
> > + OCELOT_PORT_MODE_QSGMII)
>
> There are places where OCELOT_EXT doesn't make too much sense, like here.
> The capabilities of the SERDES ports do not change depending on whether
> the switch is controlled externally or not. Same for the memory init
> delays. Maybe OCELOT_MEM_INIT_*, OCELOT_PORT_MODE_SERDES etc?
>
> There are more places as well below in function names, I'll let you be
> the judge if whether ocelot is controlled externally is relevant to what
> they do in any way.
>
> > +static int ocelot_ext_reset(struct ocelot *ocelot)
I'm looking into these changes now. I was using "ocelot_ext_" not
necessarily as an indication as "this only matters when it is controlled
externally" but rather "this is a function within ocelot_ext.c"
So there's a weird line between what constitutes "external control" vs
"generic"
There are only two things that really matter for external control:
1. The regmaps are set up in a specific way by ocelot-mfd
2. The existence of a DSA CPU port
Going by 1 only - there's basically nothing in
drivers/net/dsa/ocelot/ocelot_ext.c that is inherently "external only".
And that's kindof the point. The only thing that can be done externally
that isn't done internally would be a whole chip reset.
Going by 2 only - the simple fact that it is in drivers/net/dsa/ means
that there is a CPU port, and therefore everything in the file requires
that it is externally controlled.
Unless you're going another way, and you're not actually talking about
"function names" but instead "should this actually live in ocelot_lib"
While I don't think that's what you were directly suggesting - I like
this! ocelot_ext_reset() shouldn't exist - I should move, update, and
utilize ocelot_reset() from drivers/net/ethernet/mscc/ocelot_vsc7514.c.
The ocelot_ext function list will dwindle down to:
*_probe
*_remove
*_shutdown
*_regmap_init
*_phylink_validate
On Fri, Sep 16, 2022 at 10:31:47PM +0000, Vladimir Oltean wrote:
> On Fri, Sep 16, 2022 at 09:55:55AM -0700, Colin Foster wrote:
> Yes, please use as much as possible from the ocelot switch library,
> after all you are driving pretty much the same hardware. I'm glad for
> your revelation and sorry that I didn't think of expressing it this way
> sooner. I think the reset procedure used to be slightly different in the
> times when the ocelot_ext DSA driver also took care of setting up what
> is now the responsibility of the ocelot-mfd driver.
That's exactly right. Early on ocelot_ext was doing all sorts of things
that are now the control of ocelot-mfd and the various subsystems. These
couple procedures seem to be the last relics of those early days.
As I cleaned this up, I realized ocelot_ext_reset_phys() is no longer
needed, as it is handled in the MDIO driver. It looks like there won't
be much to remove when this is all said and done :-)
> Between then and
> now, some time has passed (years if I'm not mistaken)
Thanks for reminding me. If I knew then what I know now... maybe I'd ask
the hardware person to spec a different chip ;-)
I'm joking of course, and looking forward to being able to use this
thing!
On Fri, Sep 16, 2022 at 09:55:55AM -0700, Colin Foster wrote:
> I'm looking into these changes now. I was using "ocelot_ext_" not
> necessarily as an indication as "this only matters when it is controlled
> externally" but rather "this is a function within ocelot_ext.c"
>
> So there's a weird line between what constitutes "external control" vs
> "generic"
>
> There are only two things that really matter for external control:
> 1. The regmaps are set up in a specific way by ocelot-mfd
> 2. The existence of a DSA CPU port
>
> Going by 1 only - there's basically nothing in
> drivers/net/dsa/ocelot/ocelot_ext.c that is inherently "external only".
> And that's kindof the point. The only thing that can be done externally
> that isn't done internally would be a whole chip reset.
>
> Going by 2 only - the simple fact that it is in drivers/net/dsa/ means
> that there is a CPU port, and therefore everything in the file requires
> that it is externally controlled.
>
>
>
> Unless you're going another way, and you're not actually talking about
> "function names" but instead "should this actually live in ocelot_lib"
>
> While I don't think that's what you were directly suggesting - I like
> this! ocelot_ext_reset() shouldn't exist - I should move, update, and
> utilize ocelot_reset() from drivers/net/ethernet/mscc/ocelot_vsc7514.c.
>
> The ocelot_ext function list will dwindle down to:
> *_probe
> *_remove
> *_shutdown
> *_regmap_init
> *_phylink_validate
Yes, please use as much as possible from the ocelot switch library,
after all you are driving pretty much the same hardware. I'm glad for
your revelation and sorry that I didn't think of expressing it this way
sooner. I think the reset procedure used to be slightly different in the
times when the ocelot_ext DSA driver also took care of setting up what
is now the responsibility of the ocelot-mfd driver. Between then and
now, some time has passed (years if I'm not mistaken) and I forgot what
was and what wasn't said, I even forgot most of my own thoughts.
On Mon, Sep 12, 2022 at 05:21:10PM +0000, Vladimir Oltean wrote:
> On Sun, Sep 11, 2022 at 01:02:44PM -0700, Colin Foster wrote:
> > +#include <linux/iopoll.h>
> > +#include <linux/mfd/ocelot.h>
> > +#include <linux/phylink.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <soc/mscc/ocelot_ana.h>
> > +#include <soc/mscc/ocelot_dev.h>
> > +#include <soc/mscc/ocelot_qsys.h>
> > +#include <soc/mscc/ocelot_vcap.h>
> > +#include <soc/mscc/ocelot_ptp.h>
> > +#include <soc/mscc/ocelot_sys.h>
> > +#include <soc/mscc/ocelot.h>
> > +#include <soc/mscc/vsc7514_regs.h>
> > +#include "felix.h"
> > +
> > +#define VSC7512_NUM_PORTS 11
> > +
> > +#define OCELOT_EXT_MEM_INIT_SLEEP_US 1000
> > +#define OCELOT_EXT_MEM_INIT_TIMEOUT_US 100000
> > +
> > +#define OCELOT_EXT_PORT_MODE_SERDES (OCELOT_PORT_MODE_SGMII | \
> > + OCELOT_PORT_MODE_QSGMII)
>
> There are places where OCELOT_EXT doesn't make too much sense, like here.
> The capabilities of the SERDES ports do not change depending on whether
> the switch is controlled externally or not. Same for the memory init
> delays. Maybe OCELOT_MEM_INIT_*, OCELOT_PORT_MODE_SERDES etc?
>
> There are more places as well below in function names, I'll let you be
> the judge if whether ocelot is controlled externally is relevant to what
> they do in any way.
>
> > +static int ocelot_ext_reset(struct ocelot *ocelot)
> > +{
> > + int err, val;
> > +
> > + ocelot_ext_reset_phys(ocelot);
> > +
> > + /* Initialize chip memories */
> > + err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA], 1);
> > + if (err)
> > + return err;
> > +
> > + err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_INIT], 1);
> > + if (err)
> > + return err;
> > +
> > + /* MEM_INIT is a self-clearing bit. Wait for it to be clear (should be
> > + * 100us) before enabling the switch core
> > + */
> > + err = readx_poll_timeout(ocelot_ext_mem_init_status, ocelot, val, !val,
> > + OCELOT_EXT_MEM_INIT_SLEEP_US,
> > + OCELOT_EXT_MEM_INIT_TIMEOUT_US);
> > +
>
> I think you can eliminate the newline between the err assignment and
> checking for it.
In my upcoming v2 set, "ocelot_ext_reset" is moved to the shared
"ocelot_reset" routine. As such, iopoll.h isn't needed. And all
soc/mscc/ocelot_*.h includes aren't necessary either, since there are
literally zero register writes in ocelot_ext.c now.
I'll wait a couple days for everyone to go through their backlog. If
my "clean up ocelot_reset()" and your Documentation yaml patches get
approved, I'll be ready to send this out.