diff mbox series

[RFC,v2,net-next,2/8] net: dsa: ocelot: felix: move MDIO access to a common location

Message ID 20210710192602.2186370-3-colin.foster@in-advantage.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series Add support for VSC7511-7514 chips over SPI | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 1 maintainers not CCed: vladimir.oltean@nxp.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit fail Errors and warnings before: 0 this patch: 6
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: Improper SPDX comment style for 'drivers/net/dsa/ocelot/felix_mdio.h', please use '/*' instead WARNING: Missing or malformed SPDX-License-Identifier tag in line 1 WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/build_allmodconfig_warn fail Errors and warnings before: 0 this patch: 6
netdev/header_inline success Link

Commit Message

Colin Foster July 10, 2021, 7:25 p.m. UTC 
Indirect MDIO access is a feature that doesn't need to be specific to the
Seville driver. Separate the feature to a common file so it can be shared.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/net/dsa/ocelot/Makefile          |   1 +
 drivers/net/dsa/ocelot/felix_mdio.c      | 145 +++++++++++++++++++++++
 drivers/net/dsa/ocelot/felix_mdio.h      |  11 ++
 drivers/net/dsa/ocelot/seville_vsc9953.c | 108 ++---------------
 4 files changed, 165 insertions(+), 100 deletions(-)
 create mode 100644 drivers/net/dsa/ocelot/felix_mdio.c
 create mode 100644 drivers/net/dsa/ocelot/felix_mdio.h

Comments

Vladimir Oltean July 10, 2021, 7:59 p.m. UTC | #1 
On Sat, Jul 10, 2021 at 12:25:56PM -0700, Colin Foster wrote:
> Indirect MDIO access is a feature that doesn't need to be specific to the
> Seville driver. Separate the feature to a common file so it can be shared.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---

In fact, this same piece of hardware has a dedicated driver inside
drivers/net/mdio/mdio-mscc-miim.c. The only problem is that it doesn't
work with regmap, and especially not with a caller-supplied regmap. I
was too lazy to do that, but it is probably what should have been done.

By comparison, felix_vsc9959.c was coded up to work with an internal
MDIO bus whose ops are implemented by another driver (enetc_mdio). Maybe
you could take that as an example and have mdio-mscc-miim.c drive both
seville and ocelot.
Alexandre Belloni July 10, 2021, 8:02 p.m. UTC | #2 
On 10/07/2021 22:59:13+0300, Vladimir Oltean wrote:
> On Sat, Jul 10, 2021 at 12:25:56PM -0700, Colin Foster wrote:
> > Indirect MDIO access is a feature that doesn't need to be specific to the
> > Seville driver. Separate the feature to a common file so it can be shared.
> > 
> > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > ---
> 
> In fact, this same piece of hardware has a dedicated driver inside
> drivers/net/mdio/mdio-mscc-miim.c. The only problem is that it doesn't
> work with regmap, and especially not with a caller-supplied regmap. I
> was too lazy to do that, but it is probably what should have been done.
> 
> By comparison, felix_vsc9959.c was coded up to work with an internal
> MDIO bus whose ops are implemented by another driver (enetc_mdio). Maybe
> you could take that as an example and have mdio-mscc-miim.c drive both
> seville and ocelot.

That was indeed going to be my comment.
Colin Foster July 11, 2021, 4:41 p.m. UTC | #3 
On Sat, Jul 10, 2021 at 10:59:13PM +0300, Vladimir Oltean wrote:
> On Sat, Jul 10, 2021 at 12:25:56PM -0700, Colin Foster wrote:
> > Indirect MDIO access is a feature that doesn't need to be specific to the
> > Seville driver. Separate the feature to a common file so it can be shared.
> > 
> > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > ---
> 
> In fact, this same piece of hardware has a dedicated driver inside
> drivers/net/mdio/mdio-mscc-miim.c. The only problem is that it doesn't
> work with regmap, and especially not with a caller-supplied regmap. I
> was too lazy to do that, but it is probably what should have been done.
> 
> By comparison, felix_vsc9959.c was coded up to work with an internal
> MDIO bus whose ops are implemented by another driver (enetc_mdio). Maybe
> you could take that as an example and have mdio-mscc-miim.c drive both
> seville and ocelot.

I didn't know about that code. I'll definitely look into it.
diff mbox series

Patch

diff --git a/drivers/net/dsa/ocelot/Makefile b/drivers/net/dsa/ocelot/Makefile
index f6dd131e7491..34b9b128efb8 100644
--- a/drivers/net/dsa/ocelot/Makefile
+++ b/drivers/net/dsa/ocelot/Makefile
@@ -8,4 +8,5 @@  mscc_felix-objs := \
 
 mscc_seville-objs := \
 	felix.o \
+	felix_mdio.o \
 	seville_vsc9953.o
diff --git a/drivers/net/dsa/ocelot/felix_mdio.c b/drivers/net/dsa/ocelot/felix_mdio.c
new file mode 100644
index 000000000000..d58583311c9a
--- /dev/null
+++ b/drivers/net/dsa/ocelot/felix_mdio.c
@@ -0,0 +1,145 @@ 
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/* Distributed Switch Architecture VSC9953 driver
+ * Copyright (C) 2020, Maxim Kochetkov <fido_max@inbox.ru>
+ */
+#include <linux/types.h>
+#include <soc/mscc/ocelot.h>
+#include <linux/dsa/ocelot.h>
+#include <linux/iopoll.h>
+#include "felix.h"
+
+#define MSCC_MIIM_CMD_OPR_WRITE			BIT(1)
+#define MSCC_MIIM_CMD_OPR_READ			BIT(2)
+#define MSCC_MIIM_CMD_WRDATA_SHIFT		4
+#define MSCC_MIIM_CMD_REGAD_SHIFT		20
+#define MSCC_MIIM_CMD_PHYAD_SHIFT		25
+#define MSCC_MIIM_CMD_VLD			BIT(31)
+
+#define FELIX_MDIO_MII_TIMEOUT			10000
+#define FELIX_MDIO_MII_RETRY			10
+
+static int felix_gcb_miim_pending_status(struct ocelot *ocelot)
+{
+	int val;
+
+	ocelot_field_read(ocelot, GCB_MIIM_MII_STATUS_PENDING, &val);
+
+	return val;
+}
+
+static int felix_gcb_miim_busy_status(struct ocelot *ocelot)
+{
+	int val;
+
+	ocelot_field_read(ocelot, GCB_MIIM_MII_STATUS_BUSY, &val);
+
+	return val;
+}
+
+static int felix_mdio_write(struct mii_bus *bus, int phy_id, int regnum,
+			    u16 value)
+{
+	struct ocelot *ocelot = bus->priv;
+	int err, cmd, val;
+
+	/* Wait while MIIM controller becomes idle */
+	err = readx_poll_timeout(felix_gcb_miim_pending_status, ocelot, val,
+				 !val, FELIX_MDIO_MII_RETRY,
+				 FELIX_MDIO_MII_TIMEOUT);
+	if (err) {
+		dev_err(ocelot->dev, "MDIO write: pending timeout\n");
+		goto out;
+	}
+
+	cmd = MSCC_MIIM_CMD_VLD | (phy_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
+	      (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
+	      (value << MSCC_MIIM_CMD_WRDATA_SHIFT) |
+	      MSCC_MIIM_CMD_OPR_WRITE;
+
+	ocelot_write(ocelot, cmd, GCB_MIIM_MII_CMD);
+
+out:
+	return err;
+}
+
+static int felix_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
+{
+	struct ocelot *ocelot = bus->priv;
+	int err, cmd, val;
+
+	/* Wait until MIIM controller becomes idle */
+	err = readx_poll_timeout(felix_gcb_miim_pending_status, ocelot, val,
+				 !val, FELIX_MDIO_MII_RETRY,
+				 FELIX_MDIO_MII_TIMEOUT);
+	if (err) {
+		dev_err(ocelot->dev, "MDIO read: pending timeout\n");
+		goto out;
+	}
+
+	/* Write the MIIM COMMAND register */
+	cmd = MSCC_MIIM_CMD_VLD | (phy_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
+	      (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) | MSCC_MIIM_CMD_OPR_READ;
+
+	ocelot_write(ocelot, cmd, GCB_MIIM_MII_CMD);
+
+	/* Wait while read operation via the MIIM controller is in progress */
+	err = readx_poll_timeout(felix_gcb_miim_busy_status, ocelot, val, !val,
+				 FELIX_MDIO_MII_RETRY, FELIX_MDIO_MII_TIMEOUT);
+	if (err) {
+		dev_err(ocelot->dev, "MDIO read: busy timeout\n");
+		goto out;
+	}
+
+	val = ocelot_read(ocelot, GCB_MIIM_MII_DATA);
+
+	err = val & 0xFFFF;
+out:
+	return err;
+}
+
+int felix_mdio_register(struct ocelot *ocelot)
+{
+	struct felix *felix = ocelot_to_felix(ocelot);
+	struct device *dev = ocelot->dev;
+	int rc;
+
+	/* Needed in order to initialize the bus mutex lock */
+	rc = mdiobus_register(felix->imdio);
+	if (rc < 0) {
+		dev_err(dev, "failed to register MDIO bus\n");
+		felix->imdio = NULL;
+	}
+
+	return rc;
+}
+
+int felix_mdio_bus_alloc(struct ocelot *ocelot)
+{
+	struct felix *felix = ocelot_to_felix(ocelot);
+	struct device *dev = ocelot->dev;
+	struct mii_bus *bus;
+
+	bus = devm_mdiobus_alloc(dev);
+	if (!bus)
+		return -ENOMEM;
+
+	bus->name = "Felix internal MDIO bus";
+	bus->read = felix_mdio_read;
+	bus->write = felix_mdio_write;
+	bus->parent = dev;
+	bus->priv = ocelot;
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-imdio", dev_name(dev));
+
+	felix->imdio = bus;
+
+	return 0;
+}
+
+void felix_mdio_bus_free(struct ocelot *ocelot)
+{
+	struct felix *felix = ocelot_to_felix(ocelot);
+
+	if (felix->imdio)
+		mdiobus_unregister(felix->imdio);
+}
+
diff --git a/drivers/net/dsa/ocelot/felix_mdio.h b/drivers/net/dsa/ocelot/felix_mdio.h
new file mode 100644
index 000000000000..46fd41f605a9
--- /dev/null
+++ b/drivers/net/dsa/ocelot/felix_mdio.h
@@ -0,0 +1,11 @@ 
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/* Distributed Switch Architecture VSC9953 driver
+ * Copyright (C) 2020, Maxim Kochetkov <fido_max@inbox.ru>
+ */
+#include <linux/types.h>
+#include <soc/mscc/ocelot.h>
+
+int felix_mdio_bus_alloc(struct ocelot *ocelot);
+int felix_mdio_register(struct ocelot *ocelot);
+void felix_mdio_bus_free(struct ocelot *ocelot);
+
diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index 84f93a874d50..0e06750db264 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -11,13 +11,7 @@ 
 #include <linux/dsa/ocelot.h>
 #include <linux/iopoll.h>
 #include "felix.h"
-
-#define MSCC_MIIM_CMD_OPR_WRITE			BIT(1)
-#define MSCC_MIIM_CMD_OPR_READ			BIT(2)
-#define MSCC_MIIM_CMD_WRDATA_SHIFT		4
-#define MSCC_MIIM_CMD_REGAD_SHIFT		20
-#define MSCC_MIIM_CMD_PHYAD_SHIFT		25
-#define MSCC_MIIM_CMD_VLD			BIT(31)
+#include "felix_mdio.h"
 
 static const u32 vsc9953_ana_regmap[] = {
 	REG(ANA_ADVLEARN,			0x00b500),
@@ -857,7 +851,6 @@  static struct vcap_props vsc9953_vcap_props[] = {
 #define VSC9953_INIT_TIMEOUT			50000
 #define VSC9953_GCB_RST_SLEEP			100
 #define VSC9953_SYS_RAMINIT_SLEEP		80
-#define VCS9953_MII_TIMEOUT			10000
 
 static int vsc9953_gcb_soft_rst_status(struct ocelot *ocelot)
 {
@@ -877,82 +870,6 @@  static int vsc9953_sys_ram_init_status(struct ocelot *ocelot)
 	return val;
 }
 
-static int vsc9953_gcb_miim_pending_status(struct ocelot *ocelot)
-{
-	int val;
-
-	ocelot_field_read(ocelot, GCB_MIIM_MII_STATUS_PENDING, &val);
-
-	return val;
-}
-
-static int vsc9953_gcb_miim_busy_status(struct ocelot *ocelot)
-{
-	int val;
-
-	ocelot_field_read(ocelot, GCB_MIIM_MII_STATUS_BUSY, &val);
-
-	return val;
-}
-
-static int vsc9953_mdio_write(struct mii_bus *bus, int phy_id, int regnum,
-			      u16 value)
-{
-	struct ocelot *ocelot = bus->priv;
-	int err, cmd, val;
-
-	/* Wait while MIIM controller becomes idle */
-	err = readx_poll_timeout(vsc9953_gcb_miim_pending_status, ocelot,
-				 val, !val, 10, VCS9953_MII_TIMEOUT);
-	if (err) {
-		dev_err(ocelot->dev, "MDIO write: pending timeout\n");
-		goto out;
-	}
-
-	cmd = MSCC_MIIM_CMD_VLD | (phy_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
-	      (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
-	      (value << MSCC_MIIM_CMD_WRDATA_SHIFT) |
-	      MSCC_MIIM_CMD_OPR_WRITE;
-
-	ocelot_write(ocelot, cmd, GCB_MIIM_MII_CMD);
-
-out:
-	return err;
-}
-
-static int vsc9953_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
-{
-	struct ocelot *ocelot = bus->priv;
-	int err, cmd, val;
-
-	/* Wait until MIIM controller becomes idle */
-	err = readx_poll_timeout(vsc9953_gcb_miim_pending_status, ocelot,
-				 val, !val, 10, VCS9953_MII_TIMEOUT);
-	if (err) {
-		dev_err(ocelot->dev, "MDIO read: pending timeout\n");
-		goto out;
-	}
-
-	/* Write the MIIM COMMAND register */
-	cmd = MSCC_MIIM_CMD_VLD | (phy_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
-	      (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) | MSCC_MIIM_CMD_OPR_READ;
-
-	ocelot_write(ocelot, cmd, GCB_MIIM_MII_CMD);
-
-	/* Wait while read operation via the MIIM controller is in progress */
-	err = readx_poll_timeout(vsc9953_gcb_miim_busy_status, ocelot,
-				 val, !val, 10, VCS9953_MII_TIMEOUT);
-	if (err) {
-		dev_err(ocelot->dev, "MDIO read: busy timeout\n");
-		goto out;
-	}
-
-	val = ocelot_read(ocelot, GCB_MIIM_MII_DATA);
-
-	err = val & 0xFFFF;
-out:
-	return err;
-}
 
 /* CORE_ENA is in SYS:SYSTEM:RESET_CFG
  * MEM_INIT is in SYS:SYSTEM:RESET_CFG
@@ -1086,7 +1003,6 @@  static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
 {
 	struct felix *felix = ocelot_to_felix(ocelot);
 	struct device *dev = ocelot->dev;
-	struct mii_bus *bus;
 	int port;
 	int rc;
 
@@ -1098,26 +1014,18 @@  static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
 		return -ENOMEM;
 	}
 
-	bus = devm_mdiobus_alloc(dev);
-	if (!bus)
-		return -ENOMEM;
-
-	bus->name = "VSC9953 internal MDIO bus";
-	bus->read = vsc9953_mdio_read;
-	bus->write = vsc9953_mdio_write;
-	bus->parent = dev;
-	bus->priv = ocelot;
-	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-imdio", dev_name(dev));
+	rc = felix_mdio_bus_alloc(ocelot);
+	if (rc < 0) {
+		dev_err(dev, "failed to allocate MDIO bus\n");
+		return rc;
+	}
 
-	/* Needed in order to initialize the bus mutex lock */
-	rc = mdiobus_register(bus);
+	rc = felix_mdio_register(ocelot);
 	if (rc < 0) {
 		dev_err(dev, "failed to register MDIO bus\n");
 		return rc;
 	}
 
-	felix->imdio = bus;
-
 	for (port = 0; port < felix->info->num_ports; port++) {
 		struct ocelot_port *ocelot_port = ocelot->ports[port];
 		int addr = port + 4;
@@ -1162,7 +1070,7 @@  static void vsc9953_mdio_bus_free(struct ocelot *ocelot)
 		mdio_device_free(pcs->mdio);
 		lynx_pcs_destroy(pcs);
 	}
-	mdiobus_unregister(felix->imdio);
+	felix_mdio_bus_free(ocelot);
 }
 
 static const struct felix_info seville_info_vsc9953 = {