LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v1 net-next 0/3] update seville to use shared mdio driver
@ 2021-11-19 21:39 Colin Foster
  2021-11-19 21:39 ` [PATCH v1 net-next 1/3] net: mdio: mscc-miim: convert to a regmap implementation Colin Foster
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Colin Foster @ 2021-11-19 21:39 UTC (permalink / raw)
  To: linux-kernel, netdev
  Cc: Vladimir Oltean, Claudiu Manoil, Alexandre Belloni,
	UNGLinuxDriver, Andrew Lunn, Vivien Didelot, Florian Fainelli,
	David S. Miller, Jakub Kicinski, Heiner Kallweit, Russell King

The Seville driver uses indirect MDIO access by way of
MSCC_MIIM_CMD_* macros. This is duplicate behavior of
drivers/net/mdio/mdio-mscc-miim.c, which achieves the same thing. The
main difference being that Seville uses regmap by way of the ocelot
driver, whereas mdio-mscc-miim uses __iomem. 

This patch set converts mdio-mscc-miim to regmap, exposes an API, and
hooks Seville into this common driver by way of mscc_miim_setup.

Colin Foster (3):
  net: mdio: mscc-miim: convert to a regmap implementation
  net: dsa: ocelot: seville: utilize of_mdiobus_register
  net: dsa: ocelot: felix: switch to mdio-mscc-miim driver for indirect
    mdio access

 drivers/net/dsa/ocelot/Kconfig           |   1 +
 drivers/net/dsa/ocelot/Makefile          |   1 +
 drivers/net/dsa/ocelot/felix_mdio.c      |  54 ++++++++
 drivers/net/dsa/ocelot/felix_mdio.h      |  13 ++
 drivers/net/dsa/ocelot/seville_vsc9953.c | 107 ++-------------
 drivers/net/mdio/mdio-mscc-miim.c        | 167 +++++++++++++++++------
 include/linux/mdio/mdio-mscc-miim.h      |  19 +++
 include/soc/mscc/ocelot.h                |   1 +
 8 files changed, 220 insertions(+), 143 deletions(-)
 create mode 100644 drivers/net/dsa/ocelot/felix_mdio.c
 create mode 100644 drivers/net/dsa/ocelot/felix_mdio.h
 create mode 100644 include/linux/mdio/mdio-mscc-miim.h

-- 
2.25.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v1 net-next 1/3] net: mdio: mscc-miim: convert to a regmap implementation
  2021-11-19 21:39 [PATCH v1 net-next 0/3] update seville to use shared mdio driver Colin Foster
@ 2021-11-19 21:39 ` Colin Foster
  2021-11-20  3:56   ` Jakub Kicinski
                     ` (2 more replies)
  2021-11-19 21:39 ` [PATCH v1 net-next 2/3] net: dsa: ocelot: seville: utilize of_mdiobus_register Colin Foster
  2021-11-19 21:39 ` [PATCH v1 net-next 3/3] net: dsa: ocelot: felix: switch to mdio-mscc-miim driver for indirect mdio access Colin Foster
  2 siblings, 3 replies; 11+ messages in thread
From: Colin Foster @ 2021-11-19 21:39 UTC (permalink / raw)
  To: linux-kernel, netdev
  Cc: Vladimir Oltean, Claudiu Manoil, Alexandre Belloni,
	UNGLinuxDriver, Andrew Lunn, Vivien Didelot, Florian Fainelli,
	David S. Miller, Jakub Kicinski, Heiner Kallweit, Russell King

Utilize regmap instead of __iomem to perform indirect mdio access. This
will allow for custom regmaps to be used by way of the mscc_miim_setup
function.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/net/mdio/mdio-mscc-miim.c | 150 +++++++++++++++++++++---------
 1 file changed, 105 insertions(+), 45 deletions(-)

diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
index 17f98f609ec8..f55ad20c28d5 100644
--- a/drivers/net/mdio/mdio-mscc-miim.c
+++ b/drivers/net/mdio/mdio-mscc-miim.c
@@ -14,6 +14,7 @@
 #include <linux/of_mdio.h>
 #include <linux/phy.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 
 #define MSCC_MIIM_REG_STATUS		0x0
 #define		MSCC_MIIM_STATUS_STAT_PENDING	BIT(2)
@@ -35,37 +36,47 @@
 #define MSCC_PHY_REG_PHY_STATUS	0x4
 
 struct mscc_miim_dev {
-	void __iomem *regs;
-	void __iomem *phy_regs;
+	struct regmap *regs;
+	struct regmap *phy_regs;
 };
 
 /* When high resolution timers aren't built-in: we can't use usleep_range() as
  * we would sleep way too long. Use udelay() instead.
  */
-#define mscc_readl_poll_timeout(addr, val, cond, delay_us, timeout_us)	\
-({									\
-	if (!IS_ENABLED(CONFIG_HIGH_RES_TIMERS))			\
-		readl_poll_timeout_atomic(addr, val, cond, delay_us,	\
-					  timeout_us);			\
-	readl_poll_timeout(addr, val, cond, delay_us, timeout_us);	\
+#define mscc_readx_poll_timeout(op, addr, val, cond, delay_us, timeout_us)\
+({									  \
+	if (!IS_ENABLED(CONFIG_HIGH_RES_TIMERS))			  \
+		readx_poll_timeout_atomic(op, addr, val, cond, delay_us,  \
+					  timeout_us);			  \
+	readx_poll_timeout(op, addr, val, cond, delay_us, timeout_us);	  \
 })
 
-static int mscc_miim_wait_ready(struct mii_bus *bus)
+static int mscc_miim_status(struct mii_bus *bus)
 {
 	struct mscc_miim_dev *miim = bus->priv;
+	int val, err;
+
+	err = regmap_read(miim->regs, MSCC_MIIM_REG_STATUS, &val);
+	if (err < 0)
+		WARN_ONCE(1, "mscc miim status read error %d\n", err);
+
+	return val;
+}
+
+static int mscc_miim_wait_ready(struct mii_bus *bus)
+{
 	u32 val;
 
-	return mscc_readl_poll_timeout(miim->regs + MSCC_MIIM_REG_STATUS, val,
+	return mscc_readx_poll_timeout(mscc_miim_status, bus, val,
 				       !(val & MSCC_MIIM_STATUS_STAT_BUSY), 50,
 				       10000);
 }
 
 static int mscc_miim_wait_pending(struct mii_bus *bus)
 {
-	struct mscc_miim_dev *miim = bus->priv;
 	u32 val;
 
-	return mscc_readl_poll_timeout(miim->regs + MSCC_MIIM_REG_STATUS, val,
+	return mscc_readx_poll_timeout(mscc_miim_status, bus, val,
 				       !(val & MSCC_MIIM_STATUS_STAT_PENDING),
 				       50, 10000);
 }
@@ -73,22 +84,30 @@ static int mscc_miim_wait_pending(struct mii_bus *bus)
 static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
 {
 	struct mscc_miim_dev *miim = bus->priv;
+	int ret, err;
 	u32 val;
-	int ret;
 
 	ret = mscc_miim_wait_pending(bus);
 	if (ret)
 		goto out;
 
-	writel(MSCC_MIIM_CMD_VLD | (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
-	       (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) | MSCC_MIIM_CMD_OPR_READ,
-	       miim->regs + MSCC_MIIM_REG_CMD);
+	err = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
+			   (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
+			   (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
+			   MSCC_MIIM_CMD_OPR_READ);
+
+	if (err < 0)
+		WARN_ONCE(1, "mscc miim write cmd reg error %d\n", err);
 
 	ret = mscc_miim_wait_ready(bus);
 	if (ret)
 		goto out;
 
-	val = readl(miim->regs + MSCC_MIIM_REG_DATA);
+	err = regmap_read(miim->regs, MSCC_MIIM_REG_DATA, &val);
+
+	if (err < 0)
+		WARN_ONCE(1, "mscc miim read data reg error %d\n", err);
+
 	if (val & MSCC_MIIM_DATA_ERROR) {
 		ret = -EIO;
 		goto out;
@@ -103,18 +122,20 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
 			   int regnum, u16 value)
 {
 	struct mscc_miim_dev *miim = bus->priv;
-	int ret;
+	int err, ret;
 
 	ret = mscc_miim_wait_pending(bus);
 	if (ret < 0)
 		goto out;
 
-	writel(MSCC_MIIM_CMD_VLD | (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
-	       (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
-	       (value << MSCC_MIIM_CMD_WRDATA_SHIFT) |
-	       MSCC_MIIM_CMD_OPR_WRITE,
-	       miim->regs + MSCC_MIIM_REG_CMD);
+	err = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
+			   (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
+			   (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
+			   (value << MSCC_MIIM_CMD_WRDATA_SHIFT) |
+			   MSCC_MIIM_CMD_OPR_WRITE);
 
+	if (err < 0)
+		WARN_ONCE(1, "mscc miim write error %d\n", err);
 out:
 	return ret;
 }
@@ -122,24 +143,35 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
 static int mscc_miim_reset(struct mii_bus *bus)
 {
 	struct mscc_miim_dev *miim = bus->priv;
+	int err;
 
 	if (miim->phy_regs) {
-		writel(0, miim->phy_regs + MSCC_PHY_REG_PHY_CFG);
-		writel(0x1ff, miim->phy_regs + MSCC_PHY_REG_PHY_CFG);
+		err = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0);
+		if (err < 0)
+			WARN_ONCE(1, "mscc reset set error %d\n", err);
+
+		err = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0x1ff);
+		if (err < 0)
+			WARN_ONCE(1, "mscc reset clear error %d\n", err);
+
 		mdelay(500);
 	}
 
 	return 0;
 }
 
-static int mscc_miim_probe(struct platform_device *pdev)
+static const struct regmap_config mscc_miim_regmap_config = {
+	.reg_bits	= 32,
+	.val_bits	= 32,
+	.reg_stride	= 4,
+};
+
+static int mscc_miim_setup(struct device *dev, struct mii_bus *bus,
+			   struct regmap *mii_regmap, struct regmap *phy_regmap)
 {
-	struct mscc_miim_dev *dev;
-	struct resource *res;
-	struct mii_bus *bus;
-	int ret;
+	struct mscc_miim_dev *miim;
 
-	bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*dev));
+	bus = devm_mdiobus_alloc_size(dev, sizeof(*miim));
 	if (!bus)
 		return -ENOMEM;
 
@@ -147,26 +179,54 @@ static int mscc_miim_probe(struct platform_device *pdev)
 	bus->read = mscc_miim_read;
 	bus->write = mscc_miim_write;
 	bus->reset = mscc_miim_reset;
-	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(&pdev->dev));
-	bus->parent = &pdev->dev;
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(dev));
+	bus->parent = dev;
+
+	miim = bus->priv;
+
+	miim->regs = mii_regmap;
+	miim->phy_regs = phy_regmap;
+
+	return 0;
+}
 
-	dev = bus->priv;
-	dev->regs = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
-	if (IS_ERR(dev->regs)) {
+static int mscc_miim_probe(struct platform_device *pdev)
+{
+	struct regmap *mii_regmap, *phy_regmap;
+	void __iomem *regs, *phy_regs;
+	struct mscc_miim_dev *dev;
+	struct mii_bus *bus;
+	int ret;
+
+	regs = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
+	if (IS_ERR(regs)) {
 		dev_err(&pdev->dev, "Unable to map MIIM registers\n");
-		return PTR_ERR(dev->regs);
+		return PTR_ERR(regs);
 	}
 
-	/* This resource is optional */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	if (res) {
-		dev->phy_regs = devm_ioremap_resource(&pdev->dev, res);
-		if (IS_ERR(dev->phy_regs)) {
-			dev_err(&pdev->dev, "Unable to map internal phy registers\n");
-			return PTR_ERR(dev->phy_regs);
-		}
+	mii_regmap = devm_regmap_init_mmio(&pdev->dev, regs,
+					   &mscc_miim_regmap_config);
+
+	if (IS_ERR(mii_regmap)) {
+		dev_err(&pdev->dev, "Unable to create MIIM regmap\n");
+		return PTR_ERR(mii_regmap);
 	}
 
+	phy_regs = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(dev->phy_regs)) {
+		dev_err(&pdev->dev, "Unable to map internal phy registers\n");
+		return PTR_ERR(dev->phy_regs);
+	}
+
+	phy_regmap = devm_regmap_init_mmio(&pdev->dev, phy_regs,
+					   &mscc_miim_regmap_config);
+	if (IS_ERR(phy_regmap)) {
+		dev_err(&pdev->dev, "Unable to create phy register regmap\n");
+		return PTR_ERR(dev->phy_regs);
+	}
+
+	mscc_miim_setup(&pdev->dev, bus, mii_regmap, phy_regmap);
+
 	ret = of_mdiobus_register(bus, pdev->dev.of_node);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Cannot register MDIO bus (%d)\n", ret);
-- 
2.25.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v1 net-next 2/3] net: dsa: ocelot: seville: utilize of_mdiobus_register
  2021-11-19 21:39 [PATCH v1 net-next 0/3] update seville to use shared mdio driver Colin Foster
  2021-11-19 21:39 ` [PATCH v1 net-next 1/3] net: mdio: mscc-miim: convert to a regmap implementation Colin Foster
@ 2021-11-19 21:39 ` Colin Foster
  2021-11-19 21:39 ` [PATCH v1 net-next 3/3] net: dsa: ocelot: felix: switch to mdio-mscc-miim driver for indirect mdio access Colin Foster
  2 siblings, 0 replies; 11+ messages in thread
From: Colin Foster @ 2021-11-19 21:39 UTC (permalink / raw)
  To: linux-kernel, netdev
  Cc: Vladimir Oltean, Claudiu Manoil, Alexandre Belloni,
	UNGLinuxDriver, Andrew Lunn, Vivien Didelot, Florian Fainelli,
	David S. Miller, Jakub Kicinski, Heiner Kallweit, Russell King

Switch seville to use of_mdiobus_register(bus, NULL) instead of just
mdiobus_register. This code is about to be pulled into a separate module
that can optionally define ports by the device_node.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/ocelot/seville_vsc9953.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index 899b98193b4a..db124922c374 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -10,6 +10,7 @@
 #include <linux/pcs-lynx.h>
 #include <linux/dsa/ocelot.h>
 #include <linux/iopoll.h>
+#include <linux/of_mdio.h>
 #include "felix.h"
 
 #define MSCC_MIIM_CMD_OPR_WRITE			BIT(1)
@@ -1112,7 +1113,7 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
 	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-imdio", dev_name(dev));
 
 	/* Needed in order to initialize the bus mutex lock */
-	rc = mdiobus_register(bus);
+	rc = of_mdiobus_register(bus, NULL);
 	if (rc < 0) {
 		dev_err(dev, "failed to register MDIO bus\n");
 		return rc;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v1 net-next 3/3] net: dsa: ocelot: felix: switch to mdio-mscc-miim driver for indirect mdio access
  2021-11-19 21:39 [PATCH v1 net-next 0/3] update seville to use shared mdio driver Colin Foster
  2021-11-19 21:39 ` [PATCH v1 net-next 1/3] net: mdio: mscc-miim: convert to a regmap implementation Colin Foster
  2021-11-19 21:39 ` [PATCH v1 net-next 2/3] net: dsa: ocelot: seville: utilize of_mdiobus_register Colin Foster
@ 2021-11-19 21:39 ` Colin Foster
  2021-11-21 17:44   ` Vladimir Oltean
  2 siblings, 1 reply; 11+ messages in thread
From: Colin Foster @ 2021-11-19 21:39 UTC (permalink / raw)
  To: linux-kernel, netdev
  Cc: Vladimir Oltean, Claudiu Manoil, Alexandre Belloni,
	UNGLinuxDriver, Andrew Lunn, Vivien Didelot, Florian Fainelli,
	David S. Miller, Jakub Kicinski, Heiner Kallweit, Russell King

Switch to a shared MDIO access implementation now provided by
drivers/net/mdio/mdio-mscc-miim.c

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/net/dsa/ocelot/Kconfig           |   1 +
 drivers/net/dsa/ocelot/Makefile          |   1 +
 drivers/net/dsa/ocelot/felix_mdio.c      |  54 ++++++++++++
 drivers/net/dsa/ocelot/felix_mdio.h      |  13 +++
 drivers/net/dsa/ocelot/seville_vsc9953.c | 108 ++---------------------
 drivers/net/mdio/mdio-mscc-miim.c        |  37 ++++++--
 include/linux/mdio/mdio-mscc-miim.h      |  19 ++++
 include/soc/mscc/ocelot.h                |   1 +
 8 files changed, 125 insertions(+), 109 deletions(-)
 create mode 100644 drivers/net/dsa/ocelot/felix_mdio.c
 create mode 100644 drivers/net/dsa/ocelot/felix_mdio.h
 create mode 100644 include/linux/mdio/mdio-mscc-miim.h

diff --git a/drivers/net/dsa/ocelot/Kconfig b/drivers/net/dsa/ocelot/Kconfig
index 9948544ba1c4..220b0b027b55 100644
--- a/drivers/net/dsa/ocelot/Kconfig
+++ b/drivers/net/dsa/ocelot/Kconfig
@@ -21,6 +21,7 @@ config NET_DSA_MSCC_SEVILLE
 	depends on NET_VENDOR_MICROSEMI
 	depends on HAS_IOMEM
 	depends on PTP_1588_CLOCK_OPTIONAL
+	select MDIO_MSCC_MIIM
 	select MSCC_OCELOT_SWITCH_LIB
 	select NET_DSA_TAG_OCELOT_8021Q
 	select NET_DSA_TAG_OCELOT
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..34375285756b
--- /dev/null
+++ b/drivers/net/dsa/ocelot/felix_mdio.c
@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/* Distributed Switch Architecture VSC9953 driver
+ * Copyright (C) 2020, Maxim Kochetkov <fido_max@inbox.ru>
+ * Copyright (C) 2021 Innovative Advantage
+ */
+#include <linux/of_mdio.h>
+#include <linux/types.h>
+#include <soc/mscc/ocelot.h>
+#include <linux/dsa/ocelot.h>
+#include <linux/mdio/mdio-mscc-miim.h>
+#include "felix.h"
+#include "felix_mdio.h"
+
+int felix_of_mdio_register(struct ocelot *ocelot, struct device_node *np)
+{
+	struct felix *felix = ocelot_to_felix(ocelot);
+	struct device *dev = ocelot->dev;
+	int rc;
+
+	/* Needed in order to initialize the bus mutex lock */
+	rc = of_mdiobus_register(felix->imdio, np);
+	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;
+	int err;
+
+	err = mscc_miim_setup(dev, &bus, ocelot->targets[GCB],
+			      ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK],
+			      ocelot->targets[GCB],
+			      ocelot->map[GCB][GCB_PHY_PHY_CFG & REG_MASK]);
+
+	if (!err)
+		felix->imdio = bus;
+
+	return err;
+}
+
+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..93286f598c3b
--- /dev/null
+++ b/drivers/net/dsa/ocelot/felix_mdio.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+/* Shared code for indirect MDIO access for Felix drivers
+ *
+ * Author: Colin Foster <colin.foster@in-advantage.com>
+ * Copyright (C) 2021 Innovative Advantage
+ */
+#include <linux/of.h>
+#include <linux/types.h>
+#include <soc/mscc/ocelot.h>
+
+int felix_mdio_bus_alloc(struct ocelot *ocelot);
+int felix_of_mdio_register(struct ocelot *ocelot, struct device_node *np);
+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 db124922c374..dd7ae6a1d843 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -10,15 +10,9 @@
 #include <linux/pcs-lynx.h>
 #include <linux/dsa/ocelot.h>
 #include <linux/iopoll.h>
-#include <linux/of_mdio.h>
 #include "felix.h"
+#include "felix_mdio.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 VSC9953_VCAP_POLICER_BASE		11
 #define VSC9953_VCAP_POLICER_MAX		31
 #define VSC9953_VCAP_POLICER_BASE2		120
@@ -862,7 +856,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)
 {
@@ -882,82 +875,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
@@ -1089,7 +1006,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;
 
@@ -1101,26 +1017,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 = of_mdiobus_register(bus, NULL);
+	rc = felix_of_mdio_register(ocelot, NULL);
 	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;
@@ -1165,7 +1073,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 = {
diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
index f55ad20c28d5..cf3fa7a4459c 100644
--- a/drivers/net/mdio/mdio-mscc-miim.c
+++ b/drivers/net/mdio/mdio-mscc-miim.c
@@ -10,6 +10,7 @@
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
+#include <linux/mdio/mdio-mscc-miim.h>
 #include <linux/module.h>
 #include <linux/of_mdio.h>
 #include <linux/phy.h>
@@ -37,7 +38,9 @@
 
 struct mscc_miim_dev {
 	struct regmap *regs;
+	int mii_status_offset;
 	struct regmap *phy_regs;
+	int phy_reset_offset;
 };
 
 /* When high resolution timers aren't built-in: we can't use usleep_range() as
@@ -56,7 +59,8 @@ static int mscc_miim_status(struct mii_bus *bus)
 	struct mscc_miim_dev *miim = bus->priv;
 	int val, err;
 
-	err = regmap_read(miim->regs, MSCC_MIIM_REG_STATUS, &val);
+	err = regmap_read(miim->regs,
+			  MSCC_MIIM_REG_STATUS + miim->mii_status_offset, &val);
 	if (err < 0)
 		WARN_ONCE(1, "mscc miim status read error %d\n", err);
 
@@ -91,7 +95,9 @@ static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
 	if (ret)
 		goto out;
 
-	err = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
+	err = regmap_write(miim->regs,
+			   MSCC_MIIM_REG_CMD + miim->mii_status_offset,
+			   MSCC_MIIM_CMD_VLD |
 			   (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
 			   (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
 			   MSCC_MIIM_CMD_OPR_READ);
@@ -103,7 +109,8 @@ static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
 	if (ret)
 		goto out;
 
-	err = regmap_read(miim->regs, MSCC_MIIM_REG_DATA, &val);
+	err = regmap_read(miim->regs,
+			  MSCC_MIIM_REG_DATA + miim->mii_status_offset, &val);
 
 	if (err < 0)
 		WARN_ONCE(1, "mscc miim read data reg error %d\n", err);
@@ -128,7 +135,9 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
 	if (ret < 0)
 		goto out;
 
-	err = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
+	err = regmap_write(miim->regs,
+			   MSCC_MIIM_REG_CMD + miim->mii_status_offset,
+			   MSCC_MIIM_CMD_VLD |
 			   (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
 			   (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
 			   (value << MSCC_MIIM_CMD_WRDATA_SHIFT) |
@@ -143,14 +152,17 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
 static int mscc_miim_reset(struct mii_bus *bus)
 {
 	struct mscc_miim_dev *miim = bus->priv;
+	int offset = miim->phy_reset_offset;
 	int err;
 
 	if (miim->phy_regs) {
-		err = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0);
+		err = regmap_write(miim->phy_regs,
+				   MSCC_PHY_REG_PHY_CFG + offset, 0);
 		if (err < 0)
 			WARN_ONCE(1, "mscc reset set error %d\n", err);
 
-		err = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0x1ff);
+		err = regmap_write(miim->phy_regs,
+				   MSCC_PHY_REG_PHY_CFG + offset, 0x1ff);
 		if (err < 0)
 			WARN_ONCE(1, "mscc reset clear error %d\n", err);
 
@@ -166,10 +178,12 @@ static const struct regmap_config mscc_miim_regmap_config = {
 	.reg_stride	= 4,
 };
 
-static int mscc_miim_setup(struct device *dev, struct mii_bus *bus,
-			   struct regmap *mii_regmap, struct regmap *phy_regmap)
+int mscc_miim_setup(struct device *dev, struct mii_bus **pbus,
+		    struct regmap *mii_regmap, int status_offset,
+		    struct regmap *phy_regmap, int reset_offset)
 {
 	struct mscc_miim_dev *miim;
+	struct mii_bus *bus;
 
 	bus = devm_mdiobus_alloc_size(dev, sizeof(*miim));
 	if (!bus)
@@ -185,10 +199,15 @@ static int mscc_miim_setup(struct device *dev, struct mii_bus *bus,
 	miim = bus->priv;
 
 	miim->regs = mii_regmap;
+	miim->mii_status_offset = status_offset;
 	miim->phy_regs = phy_regmap;
+	miim->phy_reset_offset = reset_offset;
+
+	*pbus = bus;
 
 	return 0;
 }
+EXPORT_SYMBOL(mscc_miim_setup);
 
 static int mscc_miim_probe(struct platform_device *pdev)
 {
@@ -225,7 +244,7 @@ static int mscc_miim_probe(struct platform_device *pdev)
 		return PTR_ERR(dev->phy_regs);
 	}
 
-	mscc_miim_setup(&pdev->dev, bus, mii_regmap, phy_regmap);
+	mscc_miim_setup(&pdev->dev, &bus, mii_regmap, 0, phy_regmap, 0);
 
 	ret = of_mdiobus_register(bus, pdev->dev.of_node);
 	if (ret < 0) {
diff --git a/include/linux/mdio/mdio-mscc-miim.h b/include/linux/mdio/mdio-mscc-miim.h
new file mode 100644
index 000000000000..3ceab7b6ffc1
--- /dev/null
+++ b/include/linux/mdio/mdio-mscc-miim.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
+/*
+ * Driver for the MDIO interface of Microsemi network switches.
+ *
+ * Author: Colin Foster <colin.foster@in-advantage.com>
+ * Copyright (C) 2021 Innovative Advantage
+ */
+#ifndef MDIO_MSCC_MIIM_H
+#define MDIO_MSCC_MIIM_H
+
+#include <linux/device.h>
+#include <linux/phy.h>
+#include <linux/regmap.h>
+
+int mscc_miim_setup(struct device *device, struct mii_bus **bus,
+		    struct regmap *mii_regmap, int status_offset,
+		    struct regmap *phy_regmap, int reset_offset);
+
+#endif
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 89d17629efe5..9d6fe8ce9dd1 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -398,6 +398,7 @@ enum ocelot_reg {
 	GCB_MIIM_MII_STATUS,
 	GCB_MIIM_MII_CMD,
 	GCB_MIIM_MII_DATA,
+	GCB_PHY_PHY_CFG,
 	DEV_CLOCK_CFG = DEV_GMII << TARGET_OFFSET,
 	DEV_PORT_MISC,
 	DEV_EVENTS,
-- 
2.25.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 net-next 1/3] net: mdio: mscc-miim: convert to a regmap implementation
  2021-11-19 21:39 ` [PATCH v1 net-next 1/3] net: mdio: mscc-miim: convert to a regmap implementation Colin Foster
@ 2021-11-20  3:56   ` Jakub Kicinski
  2021-11-20 21:23     ` Colin Foster
  2021-11-20 15:09   ` Andrew Lunn
  2021-11-21 17:32   ` Vladimir Oltean
  2 siblings, 1 reply; 11+ messages in thread
From: Jakub Kicinski @ 2021-11-20  3:56 UTC (permalink / raw)
  To: Colin Foster
  Cc: linux-kernel, netdev, Vladimir Oltean, Claudiu Manoil,
	Alexandre Belloni, UNGLinuxDriver, Andrew Lunn, Vivien Didelot,
	Florian Fainelli, David S. Miller, Heiner Kallweit, Russell King

On Fri, 19 Nov 2021 13:39:16 -0800 Colin Foster wrote:
> Utilize regmap instead of __iomem to perform indirect mdio access. This
> will allow for custom regmaps to be used by way of the mscc_miim_setup
> function.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>

clang says:

drivers/net/mdio/mdio-mscc-miim.c:228:30: warning: variable 'bus' is uninitialized when used here [-Wuninitialized]
        mscc_miim_setup(&pdev->dev, bus, mii_regmap, phy_regmap);

drivers/net/mdio/mdio-mscc-miim.c:216:13: warning: variable 'dev' is uninitialized when used here [-Wuninitialized]
        if (IS_ERR(dev->phy_regs)) {
                   ^~~

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 net-next 1/3] net: mdio: mscc-miim: convert to a regmap implementation
  2021-11-19 21:39 ` [PATCH v1 net-next 1/3] net: mdio: mscc-miim: convert to a regmap implementation Colin Foster
  2021-11-20  3:56   ` Jakub Kicinski
@ 2021-11-20 15:09   ` Andrew Lunn
  2021-11-20 21:25     ` Colin Foster
  2021-11-21 17:32   ` Vladimir Oltean
  2 siblings, 1 reply; 11+ messages in thread
From: Andrew Lunn @ 2021-11-20 15:09 UTC (permalink / raw)
  To: Colin Foster
  Cc: linux-kernel, netdev, Vladimir Oltean, Claudiu Manoil,
	Alexandre Belloni, UNGLinuxDriver, Vivien Didelot,
	Florian Fainelli, David S. Miller, Jakub Kicinski,
	Heiner Kallweit, Russell King

> @@ -73,22 +84,30 @@ static int mscc_miim_wait_pending(struct mii_bus *bus)
>  static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
>  {
>  	struct mscc_miim_dev *miim = bus->priv;
> +	int ret, err;
>  	u32 val;
> -	int ret;
>  
>  	ret = mscc_miim_wait_pending(bus);
>  	if (ret)
>  		goto out;
>  
> -	writel(MSCC_MIIM_CMD_VLD | (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
> -	       (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) | MSCC_MIIM_CMD_OPR_READ,
> -	       miim->regs + MSCC_MIIM_REG_CMD);
> +	err = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
> +			   (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
> +			   (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
> +			   MSCC_MIIM_CMD_OPR_READ);
> +
> +	if (err < 0)
> +		WARN_ONCE(1, "mscc miim write cmd reg error %d\n", err);

You should probably return ret here. If the setup fails, i doubt you
will get anything useful from the hardware.

>  
>  	ret = mscc_miim_wait_ready(bus);
>  	if (ret)
>  		goto out;
>  
> -	val = readl(miim->regs + MSCC_MIIM_REG_DATA);
> +	err = regmap_read(miim->regs, MSCC_MIIM_REG_DATA, &val);
> +
> +	if (err < 0)
> +		WARN_ONCE(1, "mscc miim read data reg error %d\n", err);

Same here.

> +
>  	if (val & MSCC_MIIM_DATA_ERROR) {
>  		ret = -EIO;
>  		goto out;
> @@ -103,18 +122,20 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
>  			   int regnum, u16 value)
>  {
>  	struct mscc_miim_dev *miim = bus->priv;
> -	int ret;
> +	int err, ret;
>  
>  	ret = mscc_miim_wait_pending(bus);
>  	if (ret < 0)
>  		goto out;
>  
> -	writel(MSCC_MIIM_CMD_VLD | (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
> -	       (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
> -	       (value << MSCC_MIIM_CMD_WRDATA_SHIFT) |
> -	       MSCC_MIIM_CMD_OPR_WRITE,
> -	       miim->regs + MSCC_MIIM_REG_CMD);
> +	err = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
> +			   (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
> +			   (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
> +			   (value << MSCC_MIIM_CMD_WRDATA_SHIFT) |
> +			   MSCC_MIIM_CMD_OPR_WRITE);
>  
> +	if (err < 0)
> +		WARN_ONCE(1, "mscc miim write error %d\n", err);

And here, etc.

    Andrew

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 net-next 1/3] net: mdio: mscc-miim: convert to a regmap implementation
  2021-11-20  3:56   ` Jakub Kicinski
@ 2021-11-20 21:23     ` Colin Foster
  0 siblings, 0 replies; 11+ messages in thread
From: Colin Foster @ 2021-11-20 21:23 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: linux-kernel, netdev, Vladimir Oltean, Claudiu Manoil,
	Alexandre Belloni, UNGLinuxDriver, Andrew Lunn, Vivien Didelot,
	Florian Fainelli, David S. Miller, Heiner Kallweit, Russell King

Hi Jakub,

Kernel Test Robot caught this as well. I'll fix in v2. Thanks for the
feedback!

On Fri, Nov 19, 2021 at 07:56:10PM -0800, Jakub Kicinski wrote:
> On Fri, 19 Nov 2021 13:39:16 -0800 Colin Foster wrote:
> > Utilize regmap instead of __iomem to perform indirect mdio access. This
> > will allow for custom regmaps to be used by way of the mscc_miim_setup
> > function.
> > 
> > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> 
> clang says:
> 
> drivers/net/mdio/mdio-mscc-miim.c:228:30: warning: variable 'bus' is uninitialized when used here [-Wuninitialized]
>         mscc_miim_setup(&pdev->dev, bus, mii_regmap, phy_regmap);
> 
> drivers/net/mdio/mdio-mscc-miim.c:216:13: warning: variable 'dev' is uninitialized when used here [-Wuninitialized]
>         if (IS_ERR(dev->phy_regs)) {
>                    ^~~

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 net-next 1/3] net: mdio: mscc-miim: convert to a regmap implementation
  2021-11-20 15:09   ` Andrew Lunn
@ 2021-11-20 21:25     ` Colin Foster
  0 siblings, 0 replies; 11+ messages in thread
From: Colin Foster @ 2021-11-20 21:25 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: linux-kernel, netdev, Vladimir Oltean, Claudiu Manoil,
	Alexandre Belloni, UNGLinuxDriver, Vivien Didelot,
	Florian Fainelli, David S. Miller, Jakub Kicinski,
	Heiner Kallweit, Russell King

Hi Andrew,

Thank you for your feedback! I'll do a sweep for these return cases
before I submit v2.

On Sat, Nov 20, 2021 at 04:09:18PM +0100, Andrew Lunn wrote:
> > @@ -73,22 +84,30 @@ static int mscc_miim_wait_pending(struct mii_bus *bus)
> >  static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
> >  {
> >  	struct mscc_miim_dev *miim = bus->priv;
> > +	int ret, err;
> >  	u32 val;
> > -	int ret;
> >  
> >  	ret = mscc_miim_wait_pending(bus);
> >  	if (ret)
> >  		goto out;
> >  
> > -	writel(MSCC_MIIM_CMD_VLD | (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
> > -	       (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) | MSCC_MIIM_CMD_OPR_READ,
> > -	       miim->regs + MSCC_MIIM_REG_CMD);
> > +	err = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
> > +			   (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
> > +			   (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
> > +			   MSCC_MIIM_CMD_OPR_READ);
> > +
> > +	if (err < 0)
> > +		WARN_ONCE(1, "mscc miim write cmd reg error %d\n", err);
> 
> You should probably return ret here. If the setup fails, i doubt you
> will get anything useful from the hardware.
> 
> >  
> >  	ret = mscc_miim_wait_ready(bus);
> >  	if (ret)
> >  		goto out;
> >  
> > -	val = readl(miim->regs + MSCC_MIIM_REG_DATA);
> > +	err = regmap_read(miim->regs, MSCC_MIIM_REG_DATA, &val);
> > +
> > +	if (err < 0)
> > +		WARN_ONCE(1, "mscc miim read data reg error %d\n", err);
> 
> Same here.
> 
> > +
> >  	if (val & MSCC_MIIM_DATA_ERROR) {
> >  		ret = -EIO;
> >  		goto out;
> > @@ -103,18 +122,20 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
> >  			   int regnum, u16 value)
> >  {
> >  	struct mscc_miim_dev *miim = bus->priv;
> > -	int ret;
> > +	int err, ret;
> >  
> >  	ret = mscc_miim_wait_pending(bus);
> >  	if (ret < 0)
> >  		goto out;
> >  
> > -	writel(MSCC_MIIM_CMD_VLD | (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
> > -	       (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
> > -	       (value << MSCC_MIIM_CMD_WRDATA_SHIFT) |
> > -	       MSCC_MIIM_CMD_OPR_WRITE,
> > -	       miim->regs + MSCC_MIIM_REG_CMD);
> > +	err = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
> > +			   (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
> > +			   (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
> > +			   (value << MSCC_MIIM_CMD_WRDATA_SHIFT) |
> > +			   MSCC_MIIM_CMD_OPR_WRITE);
> >  
> > +	if (err < 0)
> > +		WARN_ONCE(1, "mscc miim write error %d\n", err);
> 
> And here, etc.
> 
>     Andrew

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 net-next 1/3] net: mdio: mscc-miim: convert to a regmap implementation
  2021-11-19 21:39 ` [PATCH v1 net-next 1/3] net: mdio: mscc-miim: convert to a regmap implementation Colin Foster
  2021-11-20  3:56   ` Jakub Kicinski
  2021-11-20 15:09   ` Andrew Lunn
@ 2021-11-21 17:32   ` Vladimir Oltean
  2 siblings, 0 replies; 11+ messages in thread
From: Vladimir Oltean @ 2021-11-21 17:32 UTC (permalink / raw)
  To: Colin Foster
  Cc: linux-kernel, netdev, Claudiu Manoil, Alexandre Belloni,
	UNGLinuxDriver, Andrew Lunn, Vivien Didelot, Florian Fainelli,
	David S. Miller, Jakub Kicinski, Heiner Kallweit, Russell King

On Fri, Nov 19, 2021 at 01:39:16PM -0800, Colin Foster wrote:
> Utilize regmap instead of __iomem to perform indirect mdio access. This
> will allow for custom regmaps to be used by way of the mscc_miim_setup
> function.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
>  drivers/net/mdio/mdio-mscc-miim.c | 150 +++++++++++++++++++++---------
>  1 file changed, 105 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
> index 17f98f609ec8..f55ad20c28d5 100644
> --- a/drivers/net/mdio/mdio-mscc-miim.c
> +++ b/drivers/net/mdio/mdio-mscc-miim.c
> @@ -14,6 +14,7 @@
>  #include <linux/of_mdio.h>
>  #include <linux/phy.h>
>  #include <linux/platform_device.h>
> +#include <linux/regmap.h>
>  
>  #define MSCC_MIIM_REG_STATUS		0x0
>  #define		MSCC_MIIM_STATUS_STAT_PENDING	BIT(2)
> @@ -35,37 +36,47 @@
>  #define MSCC_PHY_REG_PHY_STATUS	0x4
>  
>  struct mscc_miim_dev {
> -	void __iomem *regs;
> -	void __iomem *phy_regs;
> +	struct regmap *regs;
> +	struct regmap *phy_regs;
>  };
>  
>  /* When high resolution timers aren't built-in: we can't use usleep_range() as
>   * we would sleep way too long. Use udelay() instead.
>   */
> -#define mscc_readl_poll_timeout(addr, val, cond, delay_us, timeout_us)	\
> -({									\
> -	if (!IS_ENABLED(CONFIG_HIGH_RES_TIMERS))			\
> -		readl_poll_timeout_atomic(addr, val, cond, delay_us,	\
> -					  timeout_us);			\
> -	readl_poll_timeout(addr, val, cond, delay_us, timeout_us);	\
> +#define mscc_readx_poll_timeout(op, addr, val, cond, delay_us, timeout_us)\
> +({									  \
> +	if (!IS_ENABLED(CONFIG_HIGH_RES_TIMERS))			  \
> +		readx_poll_timeout_atomic(op, addr, val, cond, delay_us,  \
> +					  timeout_us);			  \
> +	readx_poll_timeout(op, addr, val, cond, delay_us, timeout_us);	  \
>  })
>  
> -static int mscc_miim_wait_ready(struct mii_bus *bus)
> +static int mscc_miim_status(struct mii_bus *bus)
>  {
>  	struct mscc_miim_dev *miim = bus->priv;
> +	int val, err;
> +
> +	err = regmap_read(miim->regs, MSCC_MIIM_REG_STATUS, &val);
> +	if (err < 0)
> +		WARN_ONCE(1, "mscc miim status read error %d\n", err);
> +
> +	return val;
> +}
> +
> +static int mscc_miim_wait_ready(struct mii_bus *bus)
> +{
>  	u32 val;
>  
> -	return mscc_readl_poll_timeout(miim->regs + MSCC_MIIM_REG_STATUS, val,
> +	return mscc_readx_poll_timeout(mscc_miim_status, bus, val,
>  				       !(val & MSCC_MIIM_STATUS_STAT_BUSY), 50,
>  				       10000);
>  }
>  
>  static int mscc_miim_wait_pending(struct mii_bus *bus)
>  {
> -	struct mscc_miim_dev *miim = bus->priv;
>  	u32 val;
>  
> -	return mscc_readl_poll_timeout(miim->regs + MSCC_MIIM_REG_STATUS, val,
> +	return mscc_readx_poll_timeout(mscc_miim_status, bus, val,
>  				       !(val & MSCC_MIIM_STATUS_STAT_PENDING),
>  				       50, 10000);
>  }
> @@ -73,22 +84,30 @@ static int mscc_miim_wait_pending(struct mii_bus *bus)
>  static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
>  {
>  	struct mscc_miim_dev *miim = bus->priv;
> +	int ret, err;
>  	u32 val;
> -	int ret;
>  
>  	ret = mscc_miim_wait_pending(bus);
>  	if (ret)
>  		goto out;
>  
> -	writel(MSCC_MIIM_CMD_VLD | (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
> -	       (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) | MSCC_MIIM_CMD_OPR_READ,
> -	       miim->regs + MSCC_MIIM_REG_CMD);
> +	err = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
> +			   (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
> +			   (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
> +			   MSCC_MIIM_CMD_OPR_READ);
> +
> +	if (err < 0)
> +		WARN_ONCE(1, "mscc miim write cmd reg error %d\n", err);
>  
>  	ret = mscc_miim_wait_ready(bus);
>  	if (ret)
>  		goto out;
>  
> -	val = readl(miim->regs + MSCC_MIIM_REG_DATA);
> +	err = regmap_read(miim->regs, MSCC_MIIM_REG_DATA, &val);
> +
> +	if (err < 0)
> +		WARN_ONCE(1, "mscc miim read data reg error %d\n", err);
> +
>  	if (val & MSCC_MIIM_DATA_ERROR) {
>  		ret = -EIO;
>  		goto out;
> @@ -103,18 +122,20 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
>  			   int regnum, u16 value)
>  {
>  	struct mscc_miim_dev *miim = bus->priv;
> -	int ret;
> +	int err, ret;
>  
>  	ret = mscc_miim_wait_pending(bus);
>  	if (ret < 0)
>  		goto out;
>  
> -	writel(MSCC_MIIM_CMD_VLD | (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
> -	       (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
> -	       (value << MSCC_MIIM_CMD_WRDATA_SHIFT) |
> -	       MSCC_MIIM_CMD_OPR_WRITE,
> -	       miim->regs + MSCC_MIIM_REG_CMD);
> +	err = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
> +			   (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
> +			   (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
> +			   (value << MSCC_MIIM_CMD_WRDATA_SHIFT) |
> +			   MSCC_MIIM_CMD_OPR_WRITE);
>  
> +	if (err < 0)
> +		WARN_ONCE(1, "mscc miim write error %d\n", err);
>  out:
>  	return ret;
>  }
> @@ -122,24 +143,35 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
>  static int mscc_miim_reset(struct mii_bus *bus)
>  {
>  	struct mscc_miim_dev *miim = bus->priv;
> +	int err;
>  
>  	if (miim->phy_regs) {
> -		writel(0, miim->phy_regs + MSCC_PHY_REG_PHY_CFG);
> -		writel(0x1ff, miim->phy_regs + MSCC_PHY_REG_PHY_CFG);
> +		err = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0);
> +		if (err < 0)
> +			WARN_ONCE(1, "mscc reset set error %d\n", err);
> +
> +		err = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0x1ff);
> +		if (err < 0)
> +			WARN_ONCE(1, "mscc reset clear error %d\n", err);
> +
>  		mdelay(500);
>  	}
>  
>  	return 0;
>  }
>  
> -static int mscc_miim_probe(struct platform_device *pdev)
> +static const struct regmap_config mscc_miim_regmap_config = {
> +	.reg_bits	= 32,
> +	.val_bits	= 32,
> +	.reg_stride	= 4,
> +};
> +
> +static int mscc_miim_setup(struct device *dev, struct mii_bus *bus,
> +			   struct regmap *mii_regmap, struct regmap *phy_regmap)
>  {
> -	struct mscc_miim_dev *dev;
> -	struct resource *res;
> -	struct mii_bus *bus;
> -	int ret;
> +	struct mscc_miim_dev *miim;
>  
> -	bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*dev));
> +	bus = devm_mdiobus_alloc_size(dev, sizeof(*miim));
>  	if (!bus)
>  		return -ENOMEM;
>  
> @@ -147,26 +179,54 @@ static int mscc_miim_probe(struct platform_device *pdev)
>  	bus->read = mscc_miim_read;
>  	bus->write = mscc_miim_write;
>  	bus->reset = mscc_miim_reset;
> -	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(&pdev->dev));
> -	bus->parent = &pdev->dev;
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(dev));
> +	bus->parent = dev;
> +
> +	miim = bus->priv;
> +
> +	miim->regs = mii_regmap;
> +	miim->phy_regs = phy_regmap;
> +
> +	return 0;
> +}
>  
> -	dev = bus->priv;
> -	dev->regs = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
> -	if (IS_ERR(dev->regs)) {
> +static int mscc_miim_probe(struct platform_device *pdev)
> +{
> +	struct regmap *mii_regmap, *phy_regmap;
> +	void __iomem *regs, *phy_regs;
> +	struct mscc_miim_dev *dev;
> +	struct mii_bus *bus;
> +	int ret;
> +
> +	regs = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
> +	if (IS_ERR(regs)) {
>  		dev_err(&pdev->dev, "Unable to map MIIM registers\n");
> -		return PTR_ERR(dev->regs);
> +		return PTR_ERR(regs);
>  	}
>  
> -	/* This resource is optional */
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> -	if (res) {
> -		dev->phy_regs = devm_ioremap_resource(&pdev->dev, res);
> -		if (IS_ERR(dev->phy_regs)) {
> -			dev_err(&pdev->dev, "Unable to map internal phy registers\n");
> -			return PTR_ERR(dev->phy_regs);
> -		}
> +	mii_regmap = devm_regmap_init_mmio(&pdev->dev, regs,
> +					   &mscc_miim_regmap_config);
> +
> +	if (IS_ERR(mii_regmap)) {
> +		dev_err(&pdev->dev, "Unable to create MIIM regmap\n");
> +		return PTR_ERR(mii_regmap);
>  	}
>  
> +	phy_regs = devm_platform_ioremap_resource(pdev, 1);
> +	if (IS_ERR(dev->phy_regs)) {
> +		dev_err(&pdev->dev, "Unable to map internal phy registers\n");
> +		return PTR_ERR(dev->phy_regs);
> +	}
> +
> +	phy_regmap = devm_regmap_init_mmio(&pdev->dev, phy_regs,
> +					   &mscc_miim_regmap_config);
> +	if (IS_ERR(phy_regmap)) {
> +		dev_err(&pdev->dev, "Unable to create phy register regmap\n");
> +		return PTR_ERR(dev->phy_regs);
> +	}
> +
> +	mscc_miim_setup(&pdev->dev, bus, mii_regmap, phy_regmap);

You're ignoring potential errors here.

> +
>  	ret = of_mdiobus_register(bus, pdev->dev.of_node);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "Cannot register MDIO bus (%d)\n", ret);
> -- 
> 2.25.1
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 net-next 3/3] net: dsa: ocelot: felix: switch to mdio-mscc-miim driver for indirect mdio access
  2021-11-19 21:39 ` [PATCH v1 net-next 3/3] net: dsa: ocelot: felix: switch to mdio-mscc-miim driver for indirect mdio access Colin Foster
@ 2021-11-21 17:44   ` Vladimir Oltean
  2021-11-22 17:21     ` Colin Foster
  0 siblings, 1 reply; 11+ messages in thread
From: Vladimir Oltean @ 2021-11-21 17:44 UTC (permalink / raw)
  To: Colin Foster
  Cc: linux-kernel, netdev, Claudiu Manoil, Alexandre Belloni,
	UNGLinuxDriver, Andrew Lunn, Vivien Didelot, Florian Fainelli,
	David S. Miller, Jakub Kicinski, Heiner Kallweit, Russell King

On Fri, Nov 19, 2021 at 01:39:18PM -0800, Colin Foster wrote:
> Switch to a shared MDIO access implementation now provided by
> drivers/net/mdio/mdio-mscc-miim.c
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
>  drivers/net/dsa/ocelot/Kconfig           |   1 +
>  drivers/net/dsa/ocelot/Makefile          |   1 +
>  drivers/net/dsa/ocelot/felix_mdio.c      |  54 ++++++++++++
>  drivers/net/dsa/ocelot/felix_mdio.h      |  13 +++
>  drivers/net/dsa/ocelot/seville_vsc9953.c | 108 ++---------------------
>  drivers/net/mdio/mdio-mscc-miim.c        |  37 ++++++--
>  include/linux/mdio/mdio-mscc-miim.h      |  19 ++++
>  include/soc/mscc/ocelot.h                |   1 +
>  8 files changed, 125 insertions(+), 109 deletions(-)
>  create mode 100644 drivers/net/dsa/ocelot/felix_mdio.c
>  create mode 100644 drivers/net/dsa/ocelot/felix_mdio.h
>  create mode 100644 include/linux/mdio/mdio-mscc-miim.h
> 
> diff --git a/drivers/net/dsa/ocelot/Kconfig b/drivers/net/dsa/ocelot/Kconfig
> index 9948544ba1c4..220b0b027b55 100644
> --- a/drivers/net/dsa/ocelot/Kconfig
> +++ b/drivers/net/dsa/ocelot/Kconfig
> @@ -21,6 +21,7 @@ config NET_DSA_MSCC_SEVILLE
>  	depends on NET_VENDOR_MICROSEMI
>  	depends on HAS_IOMEM
>  	depends on PTP_1588_CLOCK_OPTIONAL
> +	select MDIO_MSCC_MIIM
>  	select MSCC_OCELOT_SWITCH_LIB
>  	select NET_DSA_TAG_OCELOT_8021Q
>  	select NET_DSA_TAG_OCELOT
> 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..34375285756b
> --- /dev/null
> +++ b/drivers/net/dsa/ocelot/felix_mdio.c
> @@ -0,0 +1,54 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/* Distributed Switch Architecture VSC9953 driver
> + * Copyright (C) 2020, Maxim Kochetkov <fido_max@inbox.ru>
> + * Copyright (C) 2021 Innovative Advantage
> + */
> +#include <linux/of_mdio.h>
> +#include <linux/types.h>
> +#include <soc/mscc/ocelot.h>
> +#include <linux/dsa/ocelot.h>
> +#include <linux/mdio/mdio-mscc-miim.h>
> +#include "felix.h"
> +#include "felix_mdio.h"
> +
> +int felix_of_mdio_register(struct ocelot *ocelot, struct device_node *np)
> +{
> +	struct felix *felix = ocelot_to_felix(ocelot);
> +	struct device *dev = ocelot->dev;
> +	int rc;
> +
> +	/* Needed in order to initialize the bus mutex lock */
> +	rc = of_mdiobus_register(felix->imdio, np);
> +	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;
> +	int err;
> +
> +	err = mscc_miim_setup(dev, &bus, ocelot->targets[GCB],
> +			      ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK],
> +			      ocelot->targets[GCB],
> +			      ocelot->map[GCB][GCB_PHY_PHY_CFG & REG_MASK]);
> +
> +	if (!err)
> +		felix->imdio = bus;
> +
> +	return err;
> +}

I am a little doubtful of the value added by felix_mdio.c, since very
little is actually common. For example, the T1040 SoC has dpaa-eth
standalone ports (including the DSA master) and an embedded Seville
(VSC9953) switch. To access external PHYs connected to the switch, the
dpaa-eth MDIO bus is used (drivers/net/ethernet/freescale/xgmac_mdio.c).
The Microsemi MIIM MDIO controller from the Seville switch is used to
access only the NXP Lynx PCS, which is MDIO-mapped. That's why we put it
in felix->imdio (i == internal).

Whereas in your case, the Microsemi MIIM MDIO controller is used to
actually access the external PHYs. So it won't go in felix->imdio, since
it's not internal.

(yes, I know that NXP's integration of Vitesse switches is strange, but
it is what it is).

So unless I'm missing something, I guess you would be better off leaving
this code in Seville, and just duplicating minor portions of it (the
call to mscc_miim_setup) in your vsc7514 driver. What do you think?

> +
> +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..93286f598c3b
> --- /dev/null
> +++ b/drivers/net/dsa/ocelot/felix_mdio.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/* Shared code for indirect MDIO access for Felix drivers
> + *
> + * Author: Colin Foster <colin.foster@in-advantage.com>
> + * Copyright (C) 2021 Innovative Advantage
> + */
> +#include <linux/of.h>
> +#include <linux/types.h>
> +#include <soc/mscc/ocelot.h>
> +
> +int felix_mdio_bus_alloc(struct ocelot *ocelot);
> +int felix_of_mdio_register(struct ocelot *ocelot, struct device_node *np);
> +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 db124922c374..dd7ae6a1d843 100644
> --- a/drivers/net/dsa/ocelot/seville_vsc9953.c
> +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
> @@ -10,15 +10,9 @@
>  #include <linux/pcs-lynx.h>
>  #include <linux/dsa/ocelot.h>
>  #include <linux/iopoll.h>
> -#include <linux/of_mdio.h>
>  #include "felix.h"
> +#include "felix_mdio.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 VSC9953_VCAP_POLICER_BASE		11
>  #define VSC9953_VCAP_POLICER_MAX		31
>  #define VSC9953_VCAP_POLICER_BASE2		120
> @@ -862,7 +856,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)
>  {
> @@ -882,82 +875,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
> @@ -1089,7 +1006,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;
>  
> @@ -1101,26 +1017,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 = of_mdiobus_register(bus, NULL);
> +	rc = felix_of_mdio_register(ocelot, NULL);
>  	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;
> @@ -1165,7 +1073,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 = {
> diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
> index f55ad20c28d5..cf3fa7a4459c 100644
> --- a/drivers/net/mdio/mdio-mscc-miim.c
> +++ b/drivers/net/mdio/mdio-mscc-miim.c
> @@ -10,6 +10,7 @@
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
> +#include <linux/mdio/mdio-mscc-miim.h>
>  #include <linux/module.h>
>  #include <linux/of_mdio.h>
>  #include <linux/phy.h>
> @@ -37,7 +38,9 @@
>  
>  struct mscc_miim_dev {
>  	struct regmap *regs;
> +	int mii_status_offset;
>  	struct regmap *phy_regs;
> +	int phy_reset_offset;
>  };
>  
>  /* When high resolution timers aren't built-in: we can't use usleep_range() as
> @@ -56,7 +59,8 @@ static int mscc_miim_status(struct mii_bus *bus)
>  	struct mscc_miim_dev *miim = bus->priv;
>  	int val, err;
>  
> -	err = regmap_read(miim->regs, MSCC_MIIM_REG_STATUS, &val);
> +	err = regmap_read(miim->regs,
> +			  MSCC_MIIM_REG_STATUS + miim->mii_status_offset, &val);
>  	if (err < 0)
>  		WARN_ONCE(1, "mscc miim status read error %d\n", err);
>  
> @@ -91,7 +95,9 @@ static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
>  	if (ret)
>  		goto out;
>  
> -	err = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
> +	err = regmap_write(miim->regs,
> +			   MSCC_MIIM_REG_CMD + miim->mii_status_offset,
> +			   MSCC_MIIM_CMD_VLD |
>  			   (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
>  			   (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
>  			   MSCC_MIIM_CMD_OPR_READ);
> @@ -103,7 +109,8 @@ static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
>  	if (ret)
>  		goto out;
>  
> -	err = regmap_read(miim->regs, MSCC_MIIM_REG_DATA, &val);
> +	err = regmap_read(miim->regs,
> +			  MSCC_MIIM_REG_DATA + miim->mii_status_offset, &val);
>  
>  	if (err < 0)
>  		WARN_ONCE(1, "mscc miim read data reg error %d\n", err);
> @@ -128,7 +135,9 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
>  	if (ret < 0)
>  		goto out;
>  
> -	err = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
> +	err = regmap_write(miim->regs,
> +			   MSCC_MIIM_REG_CMD + miim->mii_status_offset,
> +			   MSCC_MIIM_CMD_VLD |
>  			   (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
>  			   (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
>  			   (value << MSCC_MIIM_CMD_WRDATA_SHIFT) |
> @@ -143,14 +152,17 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
>  static int mscc_miim_reset(struct mii_bus *bus)
>  {
>  	struct mscc_miim_dev *miim = bus->priv;
> +	int offset = miim->phy_reset_offset;
>  	int err;
>  
>  	if (miim->phy_regs) {
> -		err = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0);
> +		err = regmap_write(miim->phy_regs,
> +				   MSCC_PHY_REG_PHY_CFG + offset, 0);
>  		if (err < 0)
>  			WARN_ONCE(1, "mscc reset set error %d\n", err);
>  
> -		err = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0x1ff);
> +		err = regmap_write(miim->phy_regs,
> +				   MSCC_PHY_REG_PHY_CFG + offset, 0x1ff);
>  		if (err < 0)
>  			WARN_ONCE(1, "mscc reset clear error %d\n", err);
>  
> @@ -166,10 +178,12 @@ static const struct regmap_config mscc_miim_regmap_config = {
>  	.reg_stride	= 4,
>  };
>  
> -static int mscc_miim_setup(struct device *dev, struct mii_bus *bus,
> -			   struct regmap *mii_regmap, struct regmap *phy_regmap)
> +int mscc_miim_setup(struct device *dev, struct mii_bus **pbus,
> +		    struct regmap *mii_regmap, int status_offset,
> +		    struct regmap *phy_regmap, int reset_offset)
>  {
>  	struct mscc_miim_dev *miim;
> +	struct mii_bus *bus;
>  
>  	bus = devm_mdiobus_alloc_size(dev, sizeof(*miim));
>  	if (!bus)
> @@ -185,10 +199,15 @@ static int mscc_miim_setup(struct device *dev, struct mii_bus *bus,
>  	miim = bus->priv;
>  
>  	miim->regs = mii_regmap;
> +	miim->mii_status_offset = status_offset;
>  	miim->phy_regs = phy_regmap;
> +	miim->phy_reset_offset = reset_offset;
> +
> +	*pbus = bus;
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(mscc_miim_setup);
>  
>  static int mscc_miim_probe(struct platform_device *pdev)
>  {
> @@ -225,7 +244,7 @@ static int mscc_miim_probe(struct platform_device *pdev)
>  		return PTR_ERR(dev->phy_regs);
>  	}
>  
> -	mscc_miim_setup(&pdev->dev, bus, mii_regmap, phy_regmap);
> +	mscc_miim_setup(&pdev->dev, &bus, mii_regmap, 0, phy_regmap, 0);
>  
>  	ret = of_mdiobus_register(bus, pdev->dev.of_node);
>  	if (ret < 0) {
> diff --git a/include/linux/mdio/mdio-mscc-miim.h b/include/linux/mdio/mdio-mscc-miim.h
> new file mode 100644
> index 000000000000..3ceab7b6ffc1
> --- /dev/null
> +++ b/include/linux/mdio/mdio-mscc-miim.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +/*
> + * Driver for the MDIO interface of Microsemi network switches.
> + *
> + * Author: Colin Foster <colin.foster@in-advantage.com>
> + * Copyright (C) 2021 Innovative Advantage
> + */
> +#ifndef MDIO_MSCC_MIIM_H
> +#define MDIO_MSCC_MIIM_H
> +
> +#include <linux/device.h>
> +#include <linux/phy.h>
> +#include <linux/regmap.h>
> +
> +int mscc_miim_setup(struct device *device, struct mii_bus **bus,
> +		    struct regmap *mii_regmap, int status_offset,
> +		    struct regmap *phy_regmap, int reset_offset);
> +
> +#endif
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index 89d17629efe5..9d6fe8ce9dd1 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -398,6 +398,7 @@ enum ocelot_reg {
>  	GCB_MIIM_MII_STATUS,
>  	GCB_MIIM_MII_CMD,
>  	GCB_MIIM_MII_DATA,
> +	GCB_PHY_PHY_CFG,
>  	DEV_CLOCK_CFG = DEV_GMII << TARGET_OFFSET,
>  	DEV_PORT_MISC,
>  	DEV_EVENTS,
> -- 
> 2.25.1
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 net-next 3/3] net: dsa: ocelot: felix: switch to mdio-mscc-miim driver for indirect mdio access
  2021-11-21 17:44   ` Vladimir Oltean
@ 2021-11-22 17:21     ` Colin Foster
  0 siblings, 0 replies; 11+ messages in thread
From: Colin Foster @ 2021-11-22 17:21 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: linux-kernel, netdev, Claudiu Manoil, Alexandre Belloni,
	UNGLinuxDriver, Andrew Lunn, Vivien Didelot, Florian Fainelli,
	David S. Miller, Jakub Kicinski, Heiner Kallweit, Russell King

On Sun, Nov 21, 2021 at 05:44:13PM +0000, Vladimir Oltean wrote:
> On Fri, Nov 19, 2021 at 01:39:18PM -0800, Colin Foster wrote:
> > Switch to a shared MDIO access implementation now provided by
> > drivers/net/mdio/mdio-mscc-miim.c
> > 
> > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > ---
> >  drivers/net/dsa/ocelot/Kconfig           |   1 +
> >  drivers/net/dsa/ocelot/Makefile          |   1 +
> >  drivers/net/dsa/ocelot/felix_mdio.c      |  54 ++++++++++++
> >  drivers/net/dsa/ocelot/felix_mdio.h      |  13 +++
> >  drivers/net/dsa/ocelot/seville_vsc9953.c | 108 ++---------------------
> >  drivers/net/mdio/mdio-mscc-miim.c        |  37 ++++++--
> >  include/linux/mdio/mdio-mscc-miim.h      |  19 ++++
> >  include/soc/mscc/ocelot.h                |   1 +
> >  8 files changed, 125 insertions(+), 109 deletions(-)
> >  create mode 100644 drivers/net/dsa/ocelot/felix_mdio.c
> >  create mode 100644 drivers/net/dsa/ocelot/felix_mdio.h
> >  create mode 100644 include/linux/mdio/mdio-mscc-miim.h
> > 
> > diff --git a/drivers/net/dsa/ocelot/Kconfig b/drivers/net/dsa/ocelot/Kconfig
> > index 9948544ba1c4..220b0b027b55 100644
> > --- a/drivers/net/dsa/ocelot/Kconfig
> > +++ b/drivers/net/dsa/ocelot/Kconfig
> > @@ -21,6 +21,7 @@ config NET_DSA_MSCC_SEVILLE
> >  	depends on NET_VENDOR_MICROSEMI
> >  	depends on HAS_IOMEM
> >  	depends on PTP_1588_CLOCK_OPTIONAL
> > +	select MDIO_MSCC_MIIM
> >  	select MSCC_OCELOT_SWITCH_LIB
> >  	select NET_DSA_TAG_OCELOT_8021Q
> >  	select NET_DSA_TAG_OCELOT
> > 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..34375285756b
> > --- /dev/null
> > +++ b/drivers/net/dsa/ocelot/felix_mdio.c
> > @@ -0,0 +1,54 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/* Distributed Switch Architecture VSC9953 driver
> > + * Copyright (C) 2020, Maxim Kochetkov <fido_max@inbox.ru>
> > + * Copyright (C) 2021 Innovative Advantage
> > + */
> > +#include <linux/of_mdio.h>
> > +#include <linux/types.h>
> > +#include <soc/mscc/ocelot.h>
> > +#include <linux/dsa/ocelot.h>
> > +#include <linux/mdio/mdio-mscc-miim.h>
> > +#include "felix.h"
> > +#include "felix_mdio.h"
> > +
> > +int felix_of_mdio_register(struct ocelot *ocelot, struct device_node *np)
> > +{
> > +	struct felix *felix = ocelot_to_felix(ocelot);
> > +	struct device *dev = ocelot->dev;
> > +	int rc;
> > +
> > +	/* Needed in order to initialize the bus mutex lock */
> > +	rc = of_mdiobus_register(felix->imdio, np);
> > +	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;
> > +	int err;
> > +
> > +	err = mscc_miim_setup(dev, &bus, ocelot->targets[GCB],
> > +			      ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK],
> > +			      ocelot->targets[GCB],
> > +			      ocelot->map[GCB][GCB_PHY_PHY_CFG & REG_MASK]);
> > +
> > +	if (!err)
> > +		felix->imdio = bus;
> > +
> > +	return err;
> > +}
> 
> I am a little doubtful of the value added by felix_mdio.c, since very
> little is actually common. For example, the T1040 SoC has dpaa-eth
> standalone ports (including the DSA master) and an embedded Seville
> (VSC9953) switch. To access external PHYs connected to the switch, the
> dpaa-eth MDIO bus is used (drivers/net/ethernet/freescale/xgmac_mdio.c).
> The Microsemi MIIM MDIO controller from the Seville switch is used to
> access only the NXP Lynx PCS, which is MDIO-mapped. That's why we put it
> in felix->imdio (i == internal).

I do think some value was lost from felix_mdio.c once I was made aware
of the mscc_mdio_miim.[ch] drivers. Initially I had just pulled out
everything from vsc_9953_mdio_{read,write} into a common place... but
later it just wrapped into mscc_mdio_miim.

> 
> Whereas in your case, the Microsemi MIIM MDIO controller is used to
> actually access the external PHYs. So it won't go in felix->imdio, since
> it's not internal.

It is both actually. What is currently functional is using imdio to
access the internal copper phys to the VSC7512 on ports 0-3. My
understanding is that same bus can be used to access the phys addressed
at 4-7 on the VSC8514.

> 
> (yes, I know that NXP's integration of Vitesse switches is strange, but
> it is what it is).
> 
> So unless I'm missing something, I guess you would be better off leaving
> this code in Seville, and just duplicating minor portions of it (the
> call to mscc_miim_setup) in your vsc7514 driver. What do you think?

I think Seville could be updated to use mscc_mdio_miim, which was done
by way of felix_mdio. Maybe that's a separate patch for Seville alone.
But I have no issue with removing this file and hooking into mdio_miim
directly from the ocelot_spi driver. 

> 
> > +
> > +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..93286f598c3b
> > --- /dev/null
> > +++ b/drivers/net/dsa/ocelot/felix_mdio.h
> > @@ -0,0 +1,13 @@
> > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> > +/* Shared code for indirect MDIO access for Felix drivers
> > + *
> > + * Author: Colin Foster <colin.foster@in-advantage.com>
> > + * Copyright (C) 2021 Innovative Advantage
> > + */
> > +#include <linux/of.h>
> > +#include <linux/types.h>
> > +#include <soc/mscc/ocelot.h>
> > +
> > +int felix_mdio_bus_alloc(struct ocelot *ocelot);
> > +int felix_of_mdio_register(struct ocelot *ocelot, struct device_node *np);
> > +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 db124922c374..dd7ae6a1d843 100644
> > --- a/drivers/net/dsa/ocelot/seville_vsc9953.c
> > +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
> > @@ -10,15 +10,9 @@
> >  #include <linux/pcs-lynx.h>
> >  #include <linux/dsa/ocelot.h>
> >  #include <linux/iopoll.h>
> > -#include <linux/of_mdio.h>
> >  #include "felix.h"
> > +#include "felix_mdio.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 VSC9953_VCAP_POLICER_BASE		11
> >  #define VSC9953_VCAP_POLICER_MAX		31
> >  #define VSC9953_VCAP_POLICER_BASE2		120
> > @@ -862,7 +856,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)
> >  {
> > @@ -882,82 +875,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
> > @@ -1089,7 +1006,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;
> >  
> > @@ -1101,26 +1017,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 = of_mdiobus_register(bus, NULL);
> > +	rc = felix_of_mdio_register(ocelot, NULL);
> >  	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;
> > @@ -1165,7 +1073,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 = {
> > diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
> > index f55ad20c28d5..cf3fa7a4459c 100644
> > --- a/drivers/net/mdio/mdio-mscc-miim.c
> > +++ b/drivers/net/mdio/mdio-mscc-miim.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/io.h>
> >  #include <linux/iopoll.h>
> >  #include <linux/kernel.h>
> > +#include <linux/mdio/mdio-mscc-miim.h>
> >  #include <linux/module.h>
> >  #include <linux/of_mdio.h>
> >  #include <linux/phy.h>
> > @@ -37,7 +38,9 @@
> >  
> >  struct mscc_miim_dev {
> >  	struct regmap *regs;
> > +	int mii_status_offset;
> >  	struct regmap *phy_regs;
> > +	int phy_reset_offset;
> >  };
> >  
> >  /* When high resolution timers aren't built-in: we can't use usleep_range() as
> > @@ -56,7 +59,8 @@ static int mscc_miim_status(struct mii_bus *bus)
> >  	struct mscc_miim_dev *miim = bus->priv;
> >  	int val, err;
> >  
> > -	err = regmap_read(miim->regs, MSCC_MIIM_REG_STATUS, &val);
> > +	err = regmap_read(miim->regs,
> > +			  MSCC_MIIM_REG_STATUS + miim->mii_status_offset, &val);
> >  	if (err < 0)
> >  		WARN_ONCE(1, "mscc miim status read error %d\n", err);
> >  
> > @@ -91,7 +95,9 @@ static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
> >  	if (ret)
> >  		goto out;
> >  
> > -	err = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
> > +	err = regmap_write(miim->regs,
> > +			   MSCC_MIIM_REG_CMD + miim->mii_status_offset,
> > +			   MSCC_MIIM_CMD_VLD |
> >  			   (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
> >  			   (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
> >  			   MSCC_MIIM_CMD_OPR_READ);
> > @@ -103,7 +109,8 @@ static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
> >  	if (ret)
> >  		goto out;
> >  
> > -	err = regmap_read(miim->regs, MSCC_MIIM_REG_DATA, &val);
> > +	err = regmap_read(miim->regs,
> > +			  MSCC_MIIM_REG_DATA + miim->mii_status_offset, &val);
> >  
> >  	if (err < 0)
> >  		WARN_ONCE(1, "mscc miim read data reg error %d\n", err);
> > @@ -128,7 +135,9 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
> >  	if (ret < 0)
> >  		goto out;
> >  
> > -	err = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
> > +	err = regmap_write(miim->regs,
> > +			   MSCC_MIIM_REG_CMD + miim->mii_status_offset,
> > +			   MSCC_MIIM_CMD_VLD |
> >  			   (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
> >  			   (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
> >  			   (value << MSCC_MIIM_CMD_WRDATA_SHIFT) |
> > @@ -143,14 +152,17 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
> >  static int mscc_miim_reset(struct mii_bus *bus)
> >  {
> >  	struct mscc_miim_dev *miim = bus->priv;
> > +	int offset = miim->phy_reset_offset;
> >  	int err;
> >  
> >  	if (miim->phy_regs) {
> > -		err = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0);
> > +		err = regmap_write(miim->phy_regs,
> > +				   MSCC_PHY_REG_PHY_CFG + offset, 0);
> >  		if (err < 0)
> >  			WARN_ONCE(1, "mscc reset set error %d\n", err);
> >  
> > -		err = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0x1ff);
> > +		err = regmap_write(miim->phy_regs,
> > +				   MSCC_PHY_REG_PHY_CFG + offset, 0x1ff);
> >  		if (err < 0)
> >  			WARN_ONCE(1, "mscc reset clear error %d\n", err);
> >  
> > @@ -166,10 +178,12 @@ static const struct regmap_config mscc_miim_regmap_config = {
> >  	.reg_stride	= 4,
> >  };
> >  
> > -static int mscc_miim_setup(struct device *dev, struct mii_bus *bus,
> > -			   struct regmap *mii_regmap, struct regmap *phy_regmap)
> > +int mscc_miim_setup(struct device *dev, struct mii_bus **pbus,
> > +		    struct regmap *mii_regmap, int status_offset,
> > +		    struct regmap *phy_regmap, int reset_offset)
> >  {
> >  	struct mscc_miim_dev *miim;
> > +	struct mii_bus *bus;
> >  
> >  	bus = devm_mdiobus_alloc_size(dev, sizeof(*miim));
> >  	if (!bus)
> > @@ -185,10 +199,15 @@ static int mscc_miim_setup(struct device *dev, struct mii_bus *bus,
> >  	miim = bus->priv;
> >  
> >  	miim->regs = mii_regmap;
> > +	miim->mii_status_offset = status_offset;
> >  	miim->phy_regs = phy_regmap;
> > +	miim->phy_reset_offset = reset_offset;
> > +
> > +	*pbus = bus;
> >  
> >  	return 0;
> >  }
> > +EXPORT_SYMBOL(mscc_miim_setup);
> >  
> >  static int mscc_miim_probe(struct platform_device *pdev)
> >  {
> > @@ -225,7 +244,7 @@ static int mscc_miim_probe(struct platform_device *pdev)
> >  		return PTR_ERR(dev->phy_regs);
> >  	}
> >  
> > -	mscc_miim_setup(&pdev->dev, bus, mii_regmap, phy_regmap);
> > +	mscc_miim_setup(&pdev->dev, &bus, mii_regmap, 0, phy_regmap, 0);
> >  
> >  	ret = of_mdiobus_register(bus, pdev->dev.of_node);
> >  	if (ret < 0) {
> > diff --git a/include/linux/mdio/mdio-mscc-miim.h b/include/linux/mdio/mdio-mscc-miim.h
> > new file mode 100644
> > index 000000000000..3ceab7b6ffc1
> > --- /dev/null
> > +++ b/include/linux/mdio/mdio-mscc-miim.h
> > @@ -0,0 +1,19 @@
> > +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> > +/*
> > + * Driver for the MDIO interface of Microsemi network switches.
> > + *
> > + * Author: Colin Foster <colin.foster@in-advantage.com>
> > + * Copyright (C) 2021 Innovative Advantage
> > + */
> > +#ifndef MDIO_MSCC_MIIM_H
> > +#define MDIO_MSCC_MIIM_H
> > +
> > +#include <linux/device.h>
> > +#include <linux/phy.h>
> > +#include <linux/regmap.h>
> > +
> > +int mscc_miim_setup(struct device *device, struct mii_bus **bus,
> > +		    struct regmap *mii_regmap, int status_offset,
> > +		    struct regmap *phy_regmap, int reset_offset);
> > +
> > +#endif
> > diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> > index 89d17629efe5..9d6fe8ce9dd1 100644
> > --- a/include/soc/mscc/ocelot.h
> > +++ b/include/soc/mscc/ocelot.h
> > @@ -398,6 +398,7 @@ enum ocelot_reg {
> >  	GCB_MIIM_MII_STATUS,
> >  	GCB_MIIM_MII_CMD,
> >  	GCB_MIIM_MII_DATA,
> > +	GCB_PHY_PHY_CFG,
> >  	DEV_CLOCK_CFG = DEV_GMII << TARGET_OFFSET,
> >  	DEV_PORT_MISC,
> >  	DEV_EVENTS,
> > -- 
> > 2.25.1
> >

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-11-22 17:21 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-19 21:39 [PATCH v1 net-next 0/3] update seville to use shared mdio driver Colin Foster
2021-11-19 21:39 ` [PATCH v1 net-next 1/3] net: mdio: mscc-miim: convert to a regmap implementation Colin Foster
2021-11-20  3:56   ` Jakub Kicinski
2021-11-20 21:23     ` Colin Foster
2021-11-20 15:09   ` Andrew Lunn
2021-11-20 21:25     ` Colin Foster
2021-11-21 17:32   ` Vladimir Oltean
2021-11-19 21:39 ` [PATCH v1 net-next 2/3] net: dsa: ocelot: seville: utilize of_mdiobus_register Colin Foster
2021-11-19 21:39 ` [PATCH v1 net-next 3/3] net: dsa: ocelot: felix: switch to mdio-mscc-miim driver for indirect mdio access Colin Foster
2021-11-21 17:44   ` Vladimir Oltean
2021-11-22 17:21     ` Colin Foster

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).