[PATCH v2 net-next 0/3] update seville to use shared MDIO driver

[PATCH v2 net-next 0/3] update seville to use shared MDIO driver

[Colin Foster]

This patch set exposes and utilizes the shared MDIO bus in
drivers/net/mdio/msio-mscc-miim.c

v2:
    * Error handling (thanks Andrew Lunn)
    * Fix logic errors calling mscc_miim_setup during patch 1/3 (thanks
    Jakub Kicinski)
    * Remove unnecessary felix_mdio file (thanks Vladimir Oltean)
    * Pass NULL to mscc_miim_setup instead of GCB_PHY_PHY_CFG, since the
    phy reset isn't handled at that point of the Seville driver (patch
    3/3)

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

 drivers/net/dsa/ocelot/Kconfig           |   1 +
 drivers/net/dsa/ocelot/seville_vsc9953.c | 104 ++-----------
 drivers/net/mdio/mdio-mscc-miim.c        | 181 +++++++++++++++++------
 include/linux/mdio/mdio-mscc-miim.h      |  20 +++
 include/soc/mscc/ocelot.h                |   1 +
 5 files changed, 171 insertions(+), 136 deletions(-)
 create mode 100644 include/linux/mdio/mdio-mscc-miim.h

-- 
2.25.1

[PATCH v2 net-next 1/3] net: mdio: mscc-miim: convert to a regmap implementation

[Colin Foster]

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 | 161 ++++++++++++++++++++++--------
 1 file changed, 119 insertions(+), 42 deletions(-)

diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
index 17f98f609ec8..5a9c0b311bdb 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,49 @@
 #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, ret;
+
+	ret = regmap_read(miim->regs, MSCC_MIIM_REG_STATUS, &val);
+	if (ret < 0) {
+		WARN_ONCE(1, "mscc miim status read error %d\n", ret);
+		return ret;
+	}
+
+	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);
 }
@@ -80,15 +93,27 @@ static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
 	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);
+	ret = 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 (ret < 0) {
+		WARN_ONCE(1, "mscc miim write cmd reg error %d\n", ret);
+		goto out;
+	}
 
 	ret = mscc_miim_wait_ready(bus);
 	if (ret)
 		goto out;
 
-	val = readl(miim->regs + MSCC_MIIM_REG_DATA);
+	ret = regmap_read(miim->regs, MSCC_MIIM_REG_DATA, &val);
+
+	if (ret < 0) {
+		WARN_ONCE(1, "mscc miim read data reg error %d\n", ret);
+		goto out;
+	}
+
 	if (val & MSCC_MIIM_DATA_ERROR) {
 		ret = -EIO;
 		goto out;
@@ -109,12 +134,14 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
 	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);
+	ret = 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 (ret < 0)
+		WARN_ONCE(1, "mscc miim write error %d\n", ret);
 out:
 	return ret;
 }
@@ -122,24 +149,40 @@ 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 ret;
 
 	if (miim->phy_regs) {
-		writel(0, miim->phy_regs + MSCC_PHY_REG_PHY_CFG);
-		writel(0x1ff, miim->phy_regs + MSCC_PHY_REG_PHY_CFG);
+		ret = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0);
+		if (ret < 0) {
+			WARN_ONCE(1, "mscc reset set error %d\n", ret);
+			return ret;
+		}
+
+		ret = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0x1ff);
+		if (ret < 0) {
+			WARN_ONCE(1, "mscc reset clear error %d\n", ret);
+			return ret;
+		}
+
 		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 **pbus,
+			   struct regmap *mii_regmap, struct regmap *phy_regmap)
 {
-	struct mscc_miim_dev *dev;
-	struct resource *res;
+	struct mscc_miim_dev *miim;
 	struct mii_bus *bus;
-	int ret;
 
-	bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*dev));
+	bus = devm_mdiobus_alloc_size(dev, sizeof(*miim));
 	if (!bus)
 		return -ENOMEM;
 
@@ -147,24 +190,58 @@ 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;
+
+	*pbus = bus;
+
+	miim->regs = mii_regmap;
+	miim->phy_regs = phy_regmap;
+
+	return 0;
+}
+
+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;
 
-	dev = bus->priv;
-	dev->regs = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
-	if (IS_ERR(dev->regs)) {
+	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);
+	}
+
+	ret = mscc_miim_setup(&pdev->dev, &bus, mii_regmap, phy_regmap);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Unable to setup the MDIO bus\n");
+		return ret;
 	}
 
 	ret = of_mdiobus_register(bus, pdev->dev.of_node);
-- 
2.25.1

Re: [PATCH v2 net-next 1/3] net: mdio: mscc-miim: convert to a regmap implementation

[Colin Foster]

On Thu, Nov 25, 2021 at 12:12:59PM -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 | 161 ++++++++++++++++++++++--------
>  1 file changed, 119 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
> index 17f98f609ec8..5a9c0b311bdb 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,49 @@
>  #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, ret;
> +
> +	ret = regmap_read(miim->regs, MSCC_MIIM_REG_STATUS, &val);
> +	if (ret < 0) {
> +		WARN_ONCE(1, "mscc miim status read error %d\n", ret);
> +		return ret;
> +	}
> +
> +	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);
>  }
> @@ -80,15 +93,27 @@ static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
>  	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);
> +	ret = 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 (ret < 0) {
> +		WARN_ONCE(1, "mscc miim write cmd reg error %d\n", ret);
> +		goto out;
> +	}
>  
>  	ret = mscc_miim_wait_ready(bus);
>  	if (ret)
>  		goto out;
>  
> -	val = readl(miim->regs + MSCC_MIIM_REG_DATA);
> +	ret = regmap_read(miim->regs, MSCC_MIIM_REG_DATA, &val);
> +
> +	if (ret < 0) {
> +		WARN_ONCE(1, "mscc miim read data reg error %d\n", ret);
> +		goto out;
> +	}
> +
>  	if (val & MSCC_MIIM_DATA_ERROR) {
>  		ret = -EIO;
>  		goto out;
> @@ -109,12 +134,14 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
>  	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);
> +	ret = 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 (ret < 0)
> +		WARN_ONCE(1, "mscc miim write error %d\n", ret);
>  out:
>  	return ret;
>  }
> @@ -122,24 +149,40 @@ 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 ret;
>  
>  	if (miim->phy_regs) {
> -		writel(0, miim->phy_regs + MSCC_PHY_REG_PHY_CFG);
> -		writel(0x1ff, miim->phy_regs + MSCC_PHY_REG_PHY_CFG);
> +		ret = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0);
> +		if (ret < 0) {
> +			WARN_ONCE(1, "mscc reset set error %d\n", ret);
> +			return ret;
> +		}
> +
> +		ret = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0x1ff);
> +		if (ret < 0) {
> +			WARN_ONCE(1, "mscc reset clear error %d\n", ret);
> +			return ret;
> +		}
> +
>  		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 **pbus,
> +			   struct regmap *mii_regmap, struct regmap *phy_regmap)
>  {
> -	struct mscc_miim_dev *dev;
> -	struct resource *res;
> +	struct mscc_miim_dev *miim;
>  	struct mii_bus *bus;
> -	int ret;
>  
> -	bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*dev));
> +	bus = devm_mdiobus_alloc_size(dev, sizeof(*miim));
>  	if (!bus)
>  		return -ENOMEM;
>  
> @@ -147,24 +190,58 @@ 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;
> +
> +	*pbus = bus;
> +
> +	miim->regs = mii_regmap;
> +	miim->phy_regs = phy_regmap;
> +
> +	return 0;
> +}
> +
> +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;
>  
> -	dev = bus->priv;
> -	dev->regs = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
> -	if (IS_ERR(dev->regs)) {
> +	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)) {

Oops. I'll fix this in V3.

> +		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);
> +	}
> +
> +	ret = mscc_miim_setup(&pdev->dev, &bus, mii_regmap, phy_regmap);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Unable to setup the MDIO bus\n");
> +		return ret;
>  	}
>  
>  	ret = of_mdiobus_register(bus, pdev->dev.of_node);
> -- 
> 2.25.1
> 

Re: [PATCH v2 net-next 1/3] net: mdio: mscc-miim: convert to a regmap implementation

[Vladimir Oltean]

On Thu, Nov 25, 2021 at 12:12:59PM -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>
> ---

Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>

[PATCH v2 net-next 2/3] net: dsa: ocelot: seville: utilize of_mdiobus_register

[Colin Foster]

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

Re: [PATCH v2 net-next 2/3] net: dsa: ocelot: seville: utilize of_mdiobus_register

[Vladimir Oltean]

On Thu, Nov 25, 2021 at 12:13:00PM -0800, Colin Foster wrote:
> 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>
> ---

Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>

[PATCH v2 net-next 3/3] net: dsa: ocelot: felix: utilize shared mscc-miim driver for indirect MDIO access

[Colin Foster]

Switch to a shared MDIO access implementation by way of the mdio-mscc-miim
driver.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/net/dsa/ocelot/Kconfig           |   1 +
 drivers/net/dsa/ocelot/seville_vsc9953.c | 103 +++--------------------
 drivers/net/mdio/mdio-mscc-miim.c        |  40 ++++++---
 include/linux/mdio/mdio-mscc-miim.h      |  20 +++++
 include/soc/mscc/ocelot.h                |   1 +
 5 files changed, 61 insertions(+), 104 deletions(-)
 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/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index db124922c374..41ec60a1fc96 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -6,19 +6,14 @@
 #include <soc/mscc/ocelot_vcap.h>
 #include <soc/mscc/ocelot_sys.h>
 #include <soc/mscc/ocelot.h>
+#include <linux/mdio/mdio-mscc-miim.h>
+#include <linux/of_mdio.h>
 #include <linux/of_platform.h>
 #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)
-#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 +857,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 +876,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
@@ -1101,16 +1019,15 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
 		return -ENOMEM;
 	}
 
-	bus = devm_mdiobus_alloc(dev);
-	if (!bus)
-		return -ENOMEM;
+	rc = mscc_miim_setup(dev, &bus, "VSC9953 internal MDIO bus",
+			     ocelot->targets[GCB],
+			     ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK],
+			     NULL, 0);
 
-	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));
+	if (rc) {
+		dev_err(dev, "failed to setup MDIO bus\n");
+		return rc;
+	}
 
 	/* Needed in order to initialize the bus mutex lock */
 	rc = of_mdiobus_register(bus, NULL);
diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
index 5a9c0b311bdb..9eed6b597f21 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, ret;
 
-	ret = regmap_read(miim->regs, MSCC_MIIM_REG_STATUS, &val);
+	ret = regmap_read(miim->regs,
+			  MSCC_MIIM_REG_STATUS + miim->mii_status_offset, &val);
 	if (ret < 0) {
 		WARN_ONCE(1, "mscc miim status read error %d\n", ret);
 		return ret;
@@ -93,7 +97,9 @@ static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
 	if (ret)
 		goto out;
 
-	ret = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
+	ret = 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);
@@ -107,8 +113,8 @@ static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
 	if (ret)
 		goto out;
 
-	ret = regmap_read(miim->regs, MSCC_MIIM_REG_DATA, &val);
-
+	ret = regmap_read(miim->regs,
+			  MSCC_MIIM_REG_DATA + miim->mii_status_offset, &val);
 	if (ret < 0) {
 		WARN_ONCE(1, "mscc miim read data reg error %d\n", ret);
 		goto out;
@@ -134,7 +140,9 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
 	if (ret < 0)
 		goto out;
 
-	ret = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
+	ret = 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) |
@@ -149,16 +157,19 @@ 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 ret;
 
 	if (miim->phy_regs) {
-		ret = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0);
+		ret = regmap_write(miim->phy_regs,
+				   MSCC_PHY_REG_PHY_CFG + offset, 0);
 		if (ret < 0) {
 			WARN_ONCE(1, "mscc reset set error %d\n", ret);
 			return ret;
 		}
 
-		ret = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0x1ff);
+		ret = regmap_write(miim->phy_regs,
+				   MSCC_PHY_REG_PHY_CFG + offset, 0x1ff);
 		if (ret < 0) {
 			WARN_ONCE(1, "mscc reset clear error %d\n", ret);
 			return ret;
@@ -176,8 +187,9 @@ static const struct regmap_config mscc_miim_regmap_config = {
 	.reg_stride	= 4,
 };
 
-static int mscc_miim_setup(struct device *dev, struct mii_bus **pbus,
-			   struct regmap *mii_regmap, struct regmap *phy_regmap)
+int mscc_miim_setup(struct device *dev, struct mii_bus **pbus, const char *name,
+		    struct regmap *mii_regmap, int status_offset,
+		    struct regmap *phy_regmap, int reset_offset)
 {
 	struct mscc_miim_dev *miim;
 	struct mii_bus *bus;
@@ -186,7 +198,7 @@ static int mscc_miim_setup(struct device *dev, struct mii_bus **pbus,
 	if (!bus)
 		return -ENOMEM;
 
-	bus->name = "mscc_miim";
+	bus->name = name;
 	bus->read = mscc_miim_read;
 	bus->write = mscc_miim_write;
 	bus->reset = mscc_miim_reset;
@@ -198,10 +210,15 @@ static int mscc_miim_setup(struct device *dev, struct mii_bus **pbus,
 	*pbus = bus;
 
 	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)
 {
@@ -238,7 +255,8 @@ static int mscc_miim_probe(struct platform_device *pdev)
 		return PTR_ERR(dev->phy_regs);
 	}
 
-	ret = mscc_miim_setup(&pdev->dev, &bus, mii_regmap, phy_regmap);
+	ret = mscc_miim_setup(&pdev->dev, &bus, "mscc_miim", mii_regmap, 0,
+			      phy_regmap, 0);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Unable to setup the MDIO bus\n");
 		return ret;
diff --git a/include/linux/mdio/mdio-mscc-miim.h b/include/linux/mdio/mdio-mscc-miim.h
new file mode 100644
index 000000000000..69a023cf8991
--- /dev/null
+++ b/include/linux/mdio/mdio-mscc-miim.h
@@ -0,0 +1,20 @@
+/* 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,
+		    const char *name, 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

Re: [PATCH v2 net-next 3/3] net: dsa: ocelot: felix: utilize shared mscc-miim driver for indirect MDIO access

[Vladimir Oltean]

On Thu, Nov 25, 2021 at 12:13:01PM -0800, Colin Foster wrote:
> Switch to a shared MDIO access implementation by way of the mdio-mscc-miim
> driver.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---

I'm sorry, I still wasn't able to boot the T1040RDB to test these, it
looks like it's bricked or something. I'll try to do more debugging
tomorrow.

>  drivers/net/dsa/ocelot/Kconfig           |   1 +
>  drivers/net/dsa/ocelot/seville_vsc9953.c | 103 +++--------------------
>  drivers/net/mdio/mdio-mscc-miim.c        |  40 ++++++---
>  include/linux/mdio/mdio-mscc-miim.h      |  20 +++++
>  include/soc/mscc/ocelot.h                |   1 +
>  5 files changed, 61 insertions(+), 104 deletions(-)
>  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/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
> index db124922c374..41ec60a1fc96 100644
> --- a/drivers/net/dsa/ocelot/seville_vsc9953.c
> +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
> @@ -6,19 +6,14 @@
>  #include <soc/mscc/ocelot_vcap.h>
>  #include <soc/mscc/ocelot_sys.h>
>  #include <soc/mscc/ocelot.h>
> +#include <linux/mdio/mdio-mscc-miim.h>
> +#include <linux/of_mdio.h>
>  #include <linux/of_platform.h>
>  #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)
> -#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 +857,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 +876,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
> @@ -1101,16 +1019,15 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
>  		return -ENOMEM;
>  	}
>  
> -	bus = devm_mdiobus_alloc(dev);
> -	if (!bus)
> -		return -ENOMEM;
> +	rc = mscc_miim_setup(dev, &bus, "VSC9953 internal MDIO bus",
> +			     ocelot->targets[GCB],
> +			     ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK],
> +			     NULL, 0);
>  
> -	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));
> +	if (rc) {
> +		dev_err(dev, "failed to setup MDIO bus\n");
> +		return rc;
> +	}
>  
>  	/* Needed in order to initialize the bus mutex lock */
>  	rc = of_mdiobus_register(bus, NULL);
> diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
> index 5a9c0b311bdb..9eed6b597f21 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, ret;
>  
> -	ret = regmap_read(miim->regs, MSCC_MIIM_REG_STATUS, &val);
> +	ret = regmap_read(miim->regs,
> +			  MSCC_MIIM_REG_STATUS + miim->mii_status_offset, &val);
>  	if (ret < 0) {
>  		WARN_ONCE(1, "mscc miim status read error %d\n", ret);
>  		return ret;
> @@ -93,7 +97,9 @@ static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
>  	if (ret)
>  		goto out;
>  
> -	ret = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
> +	ret = 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);
> @@ -107,8 +113,8 @@ static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
>  	if (ret)
>  		goto out;
>  
> -	ret = regmap_read(miim->regs, MSCC_MIIM_REG_DATA, &val);
> -
> +	ret = regmap_read(miim->regs,
> +			  MSCC_MIIM_REG_DATA + miim->mii_status_offset, &val);

I'd be tempted to create one separate regmap for DEVCPU_MIIM which
starts precisely at 0x8700AC, and therefore does not need adjustment
with an offset here. What do you think?

>  	if (ret < 0) {
>  		WARN_ONCE(1, "mscc miim read data reg error %d\n", ret);
>  		goto out;
> @@ -134,7 +140,9 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
>  	if (ret < 0)
>  		goto out;
>  
> -	ret = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
> +	ret = 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) |
> @@ -149,16 +157,19 @@ 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 ret;
>  
>  	if (miim->phy_regs) {
> -		ret = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0);
> +		ret = regmap_write(miim->phy_regs,
> +				   MSCC_PHY_REG_PHY_CFG + offset, 0);
>  		if (ret < 0) {
>  			WARN_ONCE(1, "mscc reset set error %d\n", ret);
>  			return ret;
>  		}
>  
> -		ret = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0x1ff);
> +		ret = regmap_write(miim->phy_regs,
> +				   MSCC_PHY_REG_PHY_CFG + offset, 0x1ff);
>  		if (ret < 0) {
>  			WARN_ONCE(1, "mscc reset clear error %d\n", ret);
>  			return ret;
> @@ -176,8 +187,9 @@ static const struct regmap_config mscc_miim_regmap_config = {
>  	.reg_stride	= 4,
>  };
>  
> -static int mscc_miim_setup(struct device *dev, struct mii_bus **pbus,
> -			   struct regmap *mii_regmap, struct regmap *phy_regmap)
> +int mscc_miim_setup(struct device *dev, struct mii_bus **pbus, const char *name,
> +		    struct regmap *mii_regmap, int status_offset,
> +		    struct regmap *phy_regmap, int reset_offset)
>  {
>  	struct mscc_miim_dev *miim;
>  	struct mii_bus *bus;
> @@ -186,7 +198,7 @@ static int mscc_miim_setup(struct device *dev, struct mii_bus **pbus,
>  	if (!bus)
>  		return -ENOMEM;
>  
> -	bus->name = "mscc_miim";
> +	bus->name = name;
>  	bus->read = mscc_miim_read;
>  	bus->write = mscc_miim_write;
>  	bus->reset = mscc_miim_reset;
> @@ -198,10 +210,15 @@ static int mscc_miim_setup(struct device *dev, struct mii_bus **pbus,
>  	*pbus = bus;
>  
>  	miim->regs = mii_regmap;
> +	miim->mii_status_offset = status_offset;
>  	miim->phy_regs = phy_regmap;
> +	miim->phy_reset_offset = reset_offset;

The reset_offset is unused. Will vsc7514_spi need it?

> +
> +	*pbus = bus;
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(mscc_miim_setup);
>  
>  static int mscc_miim_probe(struct platform_device *pdev)
>  {
> @@ -238,7 +255,8 @@ static int mscc_miim_probe(struct platform_device *pdev)
>  		return PTR_ERR(dev->phy_regs);
>  	}
>  
> -	ret = mscc_miim_setup(&pdev->dev, &bus, mii_regmap, phy_regmap);
> +	ret = mscc_miim_setup(&pdev->dev, &bus, "mscc_miim", mii_regmap, 0,
> +			      phy_regmap, 0);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "Unable to setup the MDIO bus\n");
>  		return ret;
> diff --git a/include/linux/mdio/mdio-mscc-miim.h b/include/linux/mdio/mdio-mscc-miim.h
> new file mode 100644
> index 000000000000..69a023cf8991
> --- /dev/null
> +++ b/include/linux/mdio/mdio-mscc-miim.h
> @@ -0,0 +1,20 @@
> +/* 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,
> +		    const char *name, 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,

This appears extraneous, you are still using MSCC_PHY_REG_PHY_CFG.

>  	DEV_CLOCK_CFG = DEV_GMII << TARGET_OFFSET,
>  	DEV_PORT_MISC,
>  	DEV_EVENTS,
> -- 
> 2.25.1
>

Re: [PATCH v2 net-next 3/3] net: dsa: ocelot: felix: utilize shared mscc-miim driver for indirect MDIO access

[Colin Foster]

Hi Vladimir,

On Fri, Nov 26, 2021 at 12:58:25AM +0000, Vladimir Oltean wrote:
> On Thu, Nov 25, 2021 at 12:13:01PM -0800, Colin Foster wrote:
> > Switch to a shared MDIO access implementation by way of the mdio-mscc-miim
> > driver.
> > 
> > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > ---
> 
> I'm sorry, I still wasn't able to boot the T1040RDB to test these, it
> looks like it's bricked or something. I'll try to do more debugging
> tomorrow.

No rush - I clearly have a couple things yet to work out. I appreciate
your time!

> >  
> > -	ret = regmap_read(miim->regs, MSCC_MIIM_REG_DATA, &val);
> > -
> > +	ret = regmap_read(miim->regs,
> > +			  MSCC_MIIM_REG_DATA + miim->mii_status_offset, &val);
> 
> I'd be tempted to create one separate regmap for DEVCPU_MIIM which
> starts precisely at 0x8700AC, and therefore does not need adjustment
> with an offset here. What do you think?

I've gone back and forth on this. 

My current decision is to bring around those offset variables. I
understand it is clunky - and ends up bleeding into several drivers
(pinctrl, miim, possibly some others I haven't gotten to yet...) I'll be
the first to say I don't like this architecture.

The benefit of this is we don't have several "micro-regmaps" running
around, overlapping. 

On the other hand, maybe smaller regmaps wouldn't be the worst thing. It
might make debugging pinctrl easier if I have
sys/kernel/debug/regmap/spi0.0-ocelot_spi-devcpu-gcb-gpio insetead of
just sys/kernel/debug/regmap/spi0.0-ocelot_spi-devcpu-gcb.


So while my initial thought was "don't make extra regmaps when they
aren't needed" I'm now thinking "make extra regmaps for drivers when
they make sense." It would also make behavior consistent with how the
full VSC7514 driver acts.


The last option I haven't put much consideration toward would be to
move some of the decision making to the device tree. The main ocelot
driver appears to leave a lot of these addresses out. For instance
Documentation/devicetree/bindings/pinctrl/mscc,ocelot-pinctrl.txt. 
That added DT complexity could remove needs for lines like this:
> > +			     ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK],
But that would probably impose DT changes on Seville and Felix, which is
the last thing I want to do.



So at the end of the day, I'm now leaning toward creating a new, smaller
regmap space. It will be a proper subset of the GCB regmap. This would be 
applied here to mdio-mscc-miim, but also the pinctrl-ocelot (GCB:GPIO) and 
pinctrl-microchip-sgpio (GCB:SIO_CTRL) drivers as well for the 7512_spi
driver. I don't know of a better way to get the base address than the
code I referenced above. But I think that is probably the design I
dislike the least.


> 
> >  	if (ret < 0) {
> >  		WARN_ONCE(1, "mscc miim read data reg error %d\n", ret);
> >  		goto out;
> > @@ -134,7 +140,9 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
> >  	if (ret < 0)
> >  		goto out;
> >  
> > -	ret = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
> > +	ret = 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) |
> > @@ -149,16 +157,19 @@ 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 ret;
> >  
> >  	if (miim->phy_regs) {
> > -		ret = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0);
> > +		ret = regmap_write(miim->phy_regs,
> > +				   MSCC_PHY_REG_PHY_CFG + offset, 0);
> >  		if (ret < 0) {
> >  			WARN_ONCE(1, "mscc reset set error %d\n", ret);
> >  			return ret;
> >  		}
> >  
> > -		ret = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0x1ff);
> > +		ret = regmap_write(miim->phy_regs,
> > +				   MSCC_PHY_REG_PHY_CFG + offset, 0x1ff);
> >  		if (ret < 0) {
> >  			WARN_ONCE(1, "mscc reset clear error %d\n", ret);
> >  			return ret;
> > @@ -176,8 +187,9 @@ static const struct regmap_config mscc_miim_regmap_config = {
> >  	.reg_stride	= 4,
> >  };
> >  
> > -static int mscc_miim_setup(struct device *dev, struct mii_bus **pbus,
> > -			   struct regmap *mii_regmap, struct regmap *phy_regmap)
> > +int mscc_miim_setup(struct device *dev, struct mii_bus **pbus, const char *name,
> > +		    struct regmap *mii_regmap, int status_offset,
> > +		    struct regmap *phy_regmap, int reset_offset)
> >  {
> >  	struct mscc_miim_dev *miim;
> >  	struct mii_bus *bus;
> > @@ -186,7 +198,7 @@ static int mscc_miim_setup(struct device *dev, struct mii_bus **pbus,
> >  	if (!bus)
> >  		return -ENOMEM;
> >  
> > -	bus->name = "mscc_miim";
> > +	bus->name = name;
> >  	bus->read = mscc_miim_read;
> >  	bus->write = mscc_miim_write;
> >  	bus->reset = mscc_miim_reset;
> > @@ -198,10 +210,15 @@ static int mscc_miim_setup(struct device *dev, struct mii_bus **pbus,
> >  	*pbus = bus;
> >  
> >  	miim->regs = mii_regmap;
> > +	miim->mii_status_offset = status_offset;
> >  	miim->phy_regs = phy_regmap;
> > +	miim->phy_reset_offset = reset_offset;
> 
> The reset_offset is unused. Will vsc7514_spi need it?

Yes, the SPI driver currently uses the phy_regs regmap to reset the phys
when registering the bus. I suppose it isn't necessary to expose that
for Seville right now, since Seville didn't do resetting of the phys at
this point.

> > +	GCB_PHY_PHY_CFG,
> 
> This appears extraneous, you are still using MSCC_PHY_REG_PHY_CFG.

This is related to the comment above. They're both artifacts of the
vsc7512_spi driver and aren't currently used in Seville. For the 7512
this would get defined as 0x00f0 inside vsc7512_gcb_regmap. As suggested
way above, it sounds like the direction (for vsc7512_spi) is to create
two additional regmaps. 
One that would be GCB:MIIM. Then mdio-mscc-miim.c could refer to
GCB:MIIM:MII_CMD by way of the internal MSCC_MIIM_REG_CMD macro, as an
example. 

The same would go for MSCC_PHY_REG_PHY_CFG. If the driver is to reset
the phys during initialization, a regmap at GCB:PHY could be passed in.
Then the offsets MSCC_PHY_REG_PHY_CFG and MSCC_PHY_REG_PHY_STATUS could
be referenced.


So to summarize these changes for v3:
* Create new regmaps instead of offset variables
* Don't expose phy_regmap in mscc_miim_setup yet?
* Don't create GCB_PHY_PHY_CFG yet?

Re: [PATCH v2 net-next 3/3] net: dsa: ocelot: felix: utilize shared mscc-miim driver for indirect MDIO access

[Vladimir Oltean]

On Fri, Nov 26, 2021 at 11:58:32AM -0800, Colin Foster wrote:
> Hi Vladimir,
>
> On Fri, Nov 26, 2021 at 12:58:25AM +0000, Vladimir Oltean wrote:
> > On Thu, Nov 25, 2021 at 12:13:01PM -0800, Colin Foster wrote:
> > > Switch to a shared MDIO access implementation by way of the mdio-mscc-miim
> > > driver.
> > >
> > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > > ---
> >
> > I'm sorry, I still wasn't able to boot the T1040RDB to test these, it
> > looks like it's bricked or something. I'll try to do more debugging
> > tomorrow.
>
> No rush - I clearly have a couple things yet to work out. I appreciate
> your time!

The T1040RDB is now unbricked. Now it hangs during early kernel boot here:

[    0.010255] clocksource: timebase mult[1aaaaaab] shift[24] registered
[    0.019577] Console: colour dummy device 80x25
[    0.024016] pid_max: default: 32768 minimum: 301
[    0.028796] Mount-cache hash table entries: 8192 (order: 4, 65536 bytes, linear)
[    0.036163] Mountpoint-cache hash table entries: 8192 (order: 4, 65536 bytes, linear)
[    0.045211] e500 family performance monitor hardware support registered
[    0.051891] rcu: Hierarchical SRCU implementation.
[    0.057791] smp: Bringing up secondary CPUs ...

At this pace, I hope we'll have something by the end of the year, because
then I'll need the space in the living room for the Christmas tree :D

> > > -	ret = regmap_read(miim->regs, MSCC_MIIM_REG_DATA, &val);
> > > -
> > > +	ret = regmap_read(miim->regs,
> > > +			  MSCC_MIIM_REG_DATA + miim->mii_status_offset, &val);
> >
> > I'd be tempted to create one separate regmap for DEVCPU_MIIM which
> > starts precisely at 0x8700AC, and therefore does not need adjustment
> > with an offset here. What do you think?
>
> I've gone back and forth on this.
>
> My current decision is to bring around those offset variables. I
> understand it is clunky - and ends up bleeding into several drivers
> (pinctrl, miim, possibly some others I haven't gotten to yet...) I'll be
> the first to say I don't like this architecture.
>
> The benefit of this is we don't have several "micro-regmaps" running
> around, overlapping.
>
> On the other hand, maybe smaller regmaps wouldn't be the worst thing. It
> might make debugging pinctrl easier if I have
> sys/kernel/debug/regmap/spi0.0-ocelot_spi-devcpu-gcb-gpio insetead of
> just sys/kernel/debug/regmap/spi0.0-ocelot_spi-devcpu-gcb.
>
>
> So while my initial thought was "don't make extra regmaps when they
> aren't needed" I'm now thinking "make extra regmaps for drivers when
> they make sense." It would also make behavior consistent with how the
> full VSC7514 driver acts.

Yeah, micro-regmaps aren't the worst thing. That prize would probably go
to the T1040RDB for the pains it's putting me through.

> The last option I haven't put much consideration toward would be to
> move some of the decision making to the device tree. The main ocelot
> driver appears to leave a lot of these addresses out. For instance
> Documentation/devicetree/bindings/pinctrl/mscc,ocelot-pinctrl.txt.
> That added DT complexity could remove needs for lines like this:
> > > +			     ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK],
> But that would probably impose DT changes on Seville and Felix, which is
> the last thing I want to do.

The thing with putting the targets in the device tree is that you're
inflicting yourself unnecessary pain. Take a look at
Documentation/devicetree/bindings/net/mscc-ocelot.txt, and notice that
they mark the "ptp" target as optional because it wasn't needed when
they first published the device tree, and now they need to maintain
compatibility with those old blobs. To me that is one of the sillier
reasons why you would not support PTP, because you don't know where your
registers are. And that document is not even up to date, it hasn't been
updated when VCAP ES0, IS1, IS2 were added. I don't think that Horatiu
even bothered to maintain backwards compatibility when he initially
added tc-flower offload for VCAP IS2, and as a result, I did not bother
either when extending it for the S0 and S1 targets. At some point
afterwards, the Microchip people even stopped complaining and just went
along with it. (the story is pretty much told from memory, I'm sorry if
I mixed up some facts). It's pretty messy, and that's what you get for
creating these micro-maps of registers spread through the guts of the
SoC and then a separate reg-name for each. When we worked on the device
tree for LS1028A and then T1040, it was very much a conscious decision
for the driver to have a single, big register map and split it up pretty
much in whichever way it wants to. In fact I think we wouldn't be
having the discussion about how to split things right now if we didn't
have that flexibility.

> So at the end of the day, I'm now leaning toward creating a new, smaller
> regmap space. It will be a proper subset of the GCB regmap. This would be
> applied here to mdio-mscc-miim, but also the pinctrl-ocelot (GCB:GPIO) and
> pinctrl-microchip-sgpio (GCB:SIO_CTRL) drivers as well for the 7512_spi
> driver. I don't know of a better way to get the base address than the
> code I referenced above. But I think that is probably the design I
> dislike the least.

An offset is not all that bad TBH. I just felt like I should bring it up.
It's up to you.

> > >  	if (ret < 0) {
> > >  		WARN_ONCE(1, "mscc miim read data reg error %d\n", ret);
> > >  		goto out;
> > > @@ -134,7 +140,9 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
> > >  	if (ret < 0)
> > >  		goto out;
> > >
> > > -	ret = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
> > > +	ret = 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) |
> > > @@ -149,16 +157,19 @@ 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 ret;
> > >
> > >  	if (miim->phy_regs) {
> > > -		ret = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0);
> > > +		ret = regmap_write(miim->phy_regs,
> > > +				   MSCC_PHY_REG_PHY_CFG + offset, 0);
> > >  		if (ret < 0) {
> > >  			WARN_ONCE(1, "mscc reset set error %d\n", ret);
> > >  			return ret;
> > >  		}
> > >
> > > -		ret = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0x1ff);
> > > +		ret = regmap_write(miim->phy_regs,
> > > +				   MSCC_PHY_REG_PHY_CFG + offset, 0x1ff);
> > >  		if (ret < 0) {
> > >  			WARN_ONCE(1, "mscc reset clear error %d\n", ret);
> > >  			return ret;
> > > @@ -176,8 +187,9 @@ static const struct regmap_config mscc_miim_regmap_config = {
> > >  	.reg_stride	= 4,
> > >  };
> > >
> > > -static int mscc_miim_setup(struct device *dev, struct mii_bus **pbus,
> > > -			   struct regmap *mii_regmap, struct regmap *phy_regmap)
> > > +int mscc_miim_setup(struct device *dev, struct mii_bus **pbus, const char *name,
> > > +		    struct regmap *mii_regmap, int status_offset,
> > > +		    struct regmap *phy_regmap, int reset_offset)
> > >  {
> > >  	struct mscc_miim_dev *miim;
> > >  	struct mii_bus *bus;
> > > @@ -186,7 +198,7 @@ static int mscc_miim_setup(struct device *dev, struct mii_bus **pbus,
> > >  	if (!bus)
> > >  		return -ENOMEM;
> > >
> > > -	bus->name = "mscc_miim";
> > > +	bus->name = name;
> > >  	bus->read = mscc_miim_read;
> > >  	bus->write = mscc_miim_write;
> > >  	bus->reset = mscc_miim_reset;
> > > @@ -198,10 +210,15 @@ static int mscc_miim_setup(struct device *dev, struct mii_bus **pbus,
> > >  	*pbus = bus;
> > >
> > >  	miim->regs = mii_regmap;
> > > +	miim->mii_status_offset = status_offset;
> > >  	miim->phy_regs = phy_regmap;
> > > +	miim->phy_reset_offset = reset_offset;
> >
> > The reset_offset is unused. Will vsc7514_spi need it?
>
> Yes, the SPI driver currently uses the phy_regs regmap to reset the phys
> when registering the bus. I suppose it isn't necessary to expose that
> for Seville right now, since Seville didn't do resetting of the phys at
> this point.

Correct. One at a time.

> > > +	GCB_PHY_PHY_CFG,
> >
> > This appears extraneous, you are still using MSCC_PHY_REG_PHY_CFG.
>
> This is related to the comment above. They're both artifacts of the
> vsc7512_spi driver and aren't currently used in Seville. For the 7512
> this would get defined as 0x00f0 inside vsc7512_gcb_regmap. As suggested
> way above, it sounds like the direction (for vsc7512_spi) is to create
> two additional regmaps.
> One that would be GCB:MIIM. Then mdio-mscc-miim.c could refer to
> GCB:MIIM:MII_CMD by way of the internal MSCC_MIIM_REG_CMD macro, as an
> example.
>
> The same would go for MSCC_PHY_REG_PHY_CFG. If the driver is to reset
> the phys during initialization, a regmap at GCB:PHY could be passed in.
> Then the offsets MSCC_PHY_REG_PHY_CFG and MSCC_PHY_REG_PHY_STATUS could
> be referenced.

This could work.

> So to summarize these changes for v3:
> * Create new regmaps instead of offset variables

Optional.

> * Don't expose phy_regmap in mscc_miim_setup yet?
> * Don't create GCB_PHY_PHY_CFG yet?

Yes please.

Re: [PATCH v2 net-next 3/3] net: dsa: ocelot: felix: utilize shared mscc-miim driver for indirect MDIO access

[Vladimir Oltean]

On Thu, Nov 25, 2021 at 12:13:01PM -0800, Colin Foster wrote:
> Switch to a shared MDIO access implementation by way of the mdio-mscc-miim
> driver.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---

Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>