diff mbox series

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

Message ID 20211125201301.3748513-4-colin.foster@in-advantage.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series update seville to use shared MDIO driver | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 12 of 12 maintainers
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Colin Foster Nov. 25, 2021, 8:13 p.m. UTC 
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

Comments

Vladimir Oltean Nov. 26, 2021, 12:58 a.m. UTC | #1 
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
>
Colin Foster Nov. 26, 2021, 7:58 p.m. UTC | #2 
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?
Vladimir Oltean Nov. 26, 2021, 9:32 p.m. UTC | #3 
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.
Vladimir Oltean Nov. 27, 2021, 5:17 p.m. UTC | #4 
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>
diff mbox series

Patch

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,