[net-next,06/14] net: phy: at803x: move at8031 specific data out of generic at803x_priv

Commit Message

Christian Marangi Nov. 29, 2023, 2:12 a.m. UTC

Rework everything related to specific at8031 function to specific
function and allocate the 2 bool, is_1000basex and is_fiber and the
regulator structs to a dedicated qca8031_data struct.

This is needed to keep at803x functions more generic and detach them
from specific check of at8031/33 PHY.

Out of all the reworked functions, only config_aneg required some code
duplication with how the mdix config is handled.

This also reduces the generic at803x_priv struct by removing variables
only used by at8031 PHY.

Signed-off-by: Christian Marangi <[e-mail]>
---
 drivers/net/phy/at803x.c | 637 ++++++++++++++++++++++-----------------
 1 file changed, 362 insertions(+), 275 deletions(-)

Comments

Russell King (Oracle) Nov. 29, 2023, 9:35 a.m. UTC | #1

On Wed, Nov 29, 2023 at 03:12:11AM +0100, Christian Marangi wrote:
> Rework everything related to specific at8031 function to specific
> function and allocate the 2 bool, is_1000basex and is_fiber and the
> regulator structs to a dedicated qca8031_data struct.
> 
> This is needed to keep at803x functions more generic and detach them
> from specific check of at8031/33 PHY.
> 
> Out of all the reworked functions, only config_aneg required some code
> duplication with how the mdix config is handled.
> 
> This also reduces the generic at803x_priv struct by removing variables
> only used by at8031 PHY.

You are changing the order that register writes happen, e.g. for the
set_wol() method. at803x_set_wol() very clearly does stuff like
configuring the ethernet MAC address _before_ enabling WoL, and that
can fail. Your new code enables WoL and then calls at803x_set_wol().
If at803x_set_wol() fails (e.g. because of an invalid MAC address)
you leave WoL enabled. This is a change of behaviour.

I haven't checked anything else, but given the above, I think you
need to think more about how you make this change, and check
whether there are any other similar issues.

Christian Marangi Nov. 29, 2023, 11:08 a.m. UTC | #2

On Wed, Nov 29, 2023 at 09:35:37AM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 29, 2023 at 03:12:11AM +0100, Christian Marangi wrote:
> > Rework everything related to specific at8031 function to specific
> > function and allocate the 2 bool, is_1000basex and is_fiber and the
> > regulator structs to a dedicated qca8031_data struct.
> > 
> > This is needed to keep at803x functions more generic and detach them
> > from specific check of at8031/33 PHY.
> > 
> > Out of all the reworked functions, only config_aneg required some code
> > duplication with how the mdix config is handled.
> > 
> > This also reduces the generic at803x_priv struct by removing variables
> > only used by at8031 PHY.
> 
> You are changing the order that register writes happen, e.g. for the
> set_wol() method. at803x_set_wol() very clearly does stuff like
> configuring the ethernet MAC address _before_ enabling WoL, and that
> can fail. Your new code enables WoL and then calls at803x_set_wol().
> If at803x_set_wol() fails (e.g. because of an invalid MAC address)
> you leave WoL enabled. This is a change of behaviour.
>

Have to think about it, changing the order of the WoL module enable and
setting the MAC should not change anything as the real enablement is the
WoL interrupt. (I guess this is why the WoL module is enabled by default
as the interrupt is disabled by default resulting in the module doing
nothing)

> I haven't checked anything else, but given the above, I think you
> need to think more about how you make this change, and check
> whether there are any other similar issues.
> 

Would it be better to split this in more smaller commit? One for moving
the at8031 function and the other for refactor of some function?

Russell King (Oracle) Nov. 29, 2023, 11:31 a.m. UTC | #3

On Wed, Nov 29, 2023 at 12:08:52PM +0100, Christian Marangi wrote:
> On Wed, Nov 29, 2023 at 09:35:37AM +0000, Russell King (Oracle) wrote:
> > On Wed, Nov 29, 2023 at 03:12:11AM +0100, Christian Marangi wrote:
> > > Rework everything related to specific at8031 function to specific
> > > function and allocate the 2 bool, is_1000basex and is_fiber and the
> > > regulator structs to a dedicated qca8031_data struct.
> > > 
> > > This is needed to keep at803x functions more generic and detach them
> > > from specific check of at8031/33 PHY.
> > > 
> > > Out of all the reworked functions, only config_aneg required some code
> > > duplication with how the mdix config is handled.
> > > 
> > > This also reduces the generic at803x_priv struct by removing variables
> > > only used by at8031 PHY.
> > 
> > You are changing the order that register writes happen, e.g. for the
> > set_wol() method. at803x_set_wol() very clearly does stuff like
> > configuring the ethernet MAC address _before_ enabling WoL, and that
> > can fail. Your new code enables WoL and then calls at803x_set_wol().
> > If at803x_set_wol() fails (e.g. because of an invalid MAC address)
> > you leave WoL enabled. This is a change of behaviour.
> >
> 
> Have to think about it, changing the order of the WoL module enable and
> setting the MAC should not change anything as the real enablement is the
> WoL interrupt. (I guess this is why the WoL module is enabled by default
> as the interrupt is disabled by default resulting in the module doing
> nothing)

The AR8031 has two hardware pins for signalling WoL. One of them is the
main INT pin, which is controlled by register 0x12 bit 0. This is an
interrupt enable bit, and it only affects the INT pin.

The second is the WOL_INT pin, which is _not_ controlled by register
0x12 bit 0. This can only be controlled via the AT803X_WOL_EN in the
1588 register.

You have moved the control of AT803X_WOL_EN before the setup of the MAC,
setting and clearing it before calling the other function. This means
that if the MAC is invalid, AT803X_WOL_EN can be set, but the MAC
address has not been programmed, which can leave the machine vulnerable
to spurious wakeups if the WOL_INT pin is used for that purpose.

The original code gets the order correct. Your replacement code breaks
this ordering, thus making it less correct.

> Would it be better to split this in more smaller commit? One for moving
> the at8031 function and the other for refactor of some function? 

Given how big the series already is, you're in danger of going over the
15 patch limit for netdev submissions, so I think careful thought on
that would be needed (e.g. possibly splitting this series.) Wait until
Andrew has also reviewed the series before you decide on that though.

Andrew Lunn Nov. 30, 2023, 3:21 p.m. UTC | #4

> +struct at8031_data {
> +	bool is_fiber;
> +	bool is_1000basex;
> +	struct regulator_dev *vddio_rdev;
> +	struct regulator_dev *vddh_rdev;
> +};
> +
>  struct at803x_priv {
>  	int flags;
>  	u16 clk_25m_reg;
>  	u16 clk_25m_mask;
>  	u8 smarteee_lpi_tw_1g;
>  	u8 smarteee_lpi_tw_100m;
> -	bool is_fiber;
> -	bool is_1000basex;
> -	struct regulator_dev *vddio_rdev;
> -	struct regulator_dev *vddh_rdev;
> +
> +	/* Specific data for at8031 PHYs */
> +	void *data;
>  };

I don't really like this void *

Go through at803x_priv and find out what is common to them all, and
keep that in one structure. Add per family private structures which
include the common as a member.

By having real types everywhere you get the compiler doing checks for
you.

As Russell pointed out, this patch series is going to be too big. So
break it up. We can move fast on patches which are simple and
obviously correct.

	  Andrew

Christian Marangi Nov. 30, 2023, 7:38 p.m. UTC | #5

On Thu, Nov 30, 2023 at 04:21:50PM +0100, Andrew Lunn wrote:
> > +struct at8031_data {
> > +	bool is_fiber;
> > +	bool is_1000basex;
> > +	struct regulator_dev *vddio_rdev;
> > +	struct regulator_dev *vddh_rdev;
> > +};
> > +
> >  struct at803x_priv {
> >  	int flags;
> >  	u16 clk_25m_reg;
> >  	u16 clk_25m_mask;
> >  	u8 smarteee_lpi_tw_1g;
> >  	u8 smarteee_lpi_tw_100m;
> > -	bool is_fiber;
> > -	bool is_1000basex;
> > -	struct regulator_dev *vddio_rdev;
> > -	struct regulator_dev *vddh_rdev;
> > +
> > +	/* Specific data for at8031 PHYs */
> > +	void *data;
> >  };
> 
> I don't really like this void *
> 
> Go through at803x_priv and find out what is common to them all, and
> keep that in one structure. Add per family private structures which
> include the common as a member.

As you notice later in the patches, only at803x have stuff in common
qca803xx and qca808x doesn't use the struct at all (aside from stats)

And in the at803x PHY family only at8031 have fiber 1000basex and
regulators.

> 
> By having real types everywhere you get the compiler doing checks for
> you.

Main problem is that adding something like
'struct at8031_data* at8031_data' looks also bad.

Maybe I can rework the 2 bool to flags (they are used only by at803x)
and keep the 2 regulator pointer?

> 
> As Russell pointed out, this patch series is going to be too big. So
> break it up. We can move fast on patches which are simple and
> obviously correct.
>

Andrew Lunn Nov. 30, 2023, 8:14 p.m. UTC | #6

On Thu, Nov 30, 2023 at 08:38:17PM +0100, Christian Marangi wrote:
> On Thu, Nov 30, 2023 at 04:21:50PM +0100, Andrew Lunn wrote:
> > > +struct at8031_data {
> > > +	bool is_fiber;
> > > +	bool is_1000basex;
> > > +	struct regulator_dev *vddio_rdev;
> > > +	struct regulator_dev *vddh_rdev;
> > > +};
> > > +
> > >  struct at803x_priv {
> > >  	int flags;
> > >  	u16 clk_25m_reg;
> > >  	u16 clk_25m_mask;
> > >  	u8 smarteee_lpi_tw_1g;
> > >  	u8 smarteee_lpi_tw_100m;
> > > -	bool is_fiber;
> > > -	bool is_1000basex;
> > > -	struct regulator_dev *vddio_rdev;
> > > -	struct regulator_dev *vddh_rdev;
> > > +
> > > +	/* Specific data for at8031 PHYs */
> > > +	void *data;
> > >  };
> > 
> > I don't really like this void *
> > 
> > Go through at803x_priv and find out what is common to them all, and
> > keep that in one structure. Add per family private structures which
> > include the common as a member.
> 
> As you notice later in the patches, only at803x have stuff in common
> qca803xx and qca808x doesn't use the struct at all (aside from stats)

The dangers here are taking a phydev->priv and casting it. You think
it is X, but is actually Y, and bad things happen.

The helpers you have in your common.c must never do this. You can have
a at803x_priv only visible inside the at803x driver, and a
qca808x_priv only visible inside the qca808x driver. Define a
structure which is needed for the shared code in common.c, and pass it
as a parameter to these helpers.

You have a reasonably good idea what your end goal is. The tricky part
is getting there, in lots of easy to review, obviously correct steps.

	Andrew

Christian Marangi Nov. 30, 2023, 8:24 p.m. UTC | #7

On Thu, Nov 30, 2023 at 09:14:00PM +0100, Andrew Lunn wrote:
> On Thu, Nov 30, 2023 at 08:38:17PM +0100, Christian Marangi wrote:
> > On Thu, Nov 30, 2023 at 04:21:50PM +0100, Andrew Lunn wrote:
> > > > +struct at8031_data {
> > > > +	bool is_fiber;
> > > > +	bool is_1000basex;
> > > > +	struct regulator_dev *vddio_rdev;
> > > > +	struct regulator_dev *vddh_rdev;
> > > > +};
> > > > +
> > > >  struct at803x_priv {
> > > >  	int flags;
> > > >  	u16 clk_25m_reg;
> > > >  	u16 clk_25m_mask;
> > > >  	u8 smarteee_lpi_tw_1g;
> > > >  	u8 smarteee_lpi_tw_100m;
> > > > -	bool is_fiber;
> > > > -	bool is_1000basex;
> > > > -	struct regulator_dev *vddio_rdev;
> > > > -	struct regulator_dev *vddh_rdev;
> > > > +
> > > > +	/* Specific data for at8031 PHYs */
> > > > +	void *data;
> > > >  };
> > > 
> > > I don't really like this void *
> > > 
> > > Go through at803x_priv and find out what is common to them all, and
> > > keep that in one structure. Add per family private structures which
> > > include the common as a member.
> > 
> > As you notice later in the patches, only at803x have stuff in common
> > qca803xx and qca808x doesn't use the struct at all (aside from stats)
> 
> The dangers here are taking a phydev->priv and casting it. You think
> it is X, but is actually Y, and bad things happen.
> 
> The helpers you have in your common.c must never do this. You can have
> a at803x_priv only visible inside the at803x driver, and a
> qca808x_priv only visible inside the qca808x driver. Define a
> structure which is needed for the shared code in common.c, and pass it
> as a parameter to these helpers.

Tell me if the idea is crazy enough. Ideally common function should do
simple phy read/write and should not reference stuff using priv (as we
would have the problem you are pointing out)

But phy_read/write needs phydev...

Would be ok to have something like

struct qca_ethphy_common {
 struct phy_device *phydev;
}

And pass this struct to the helper? Is it enough to desist devs from
starting introducing function in common.c, checking the ID there and
starting doing stuff with funny specific phydev priv?

> 
> You have a reasonably good idea what your end goal is. The tricky part
> is getting there, in lots of easy to review, obviously correct steps.
>

Patch

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 9a590124d1fe..b83422c6db74 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -305,16 +305,22 @@  struct qca83xx_priv {
 	u64 stats[ARRAY_SIZE(qca83xx_hw_stats)];
 };
 
+struct at8031_data {
+	bool is_fiber;
+	bool is_1000basex;
+	struct regulator_dev *vddio_rdev;
+	struct regulator_dev *vddh_rdev;
+};
+
 struct at803x_priv {
 	int flags;
 	u16 clk_25m_reg;
 	u16 clk_25m_mask;
 	u8 smarteee_lpi_tw_1g;
 	u8 smarteee_lpi_tw_100m;
-	bool is_fiber;
-	bool is_1000basex;
-	struct regulator_dev *vddio_rdev;
-	struct regulator_dev *vddh_rdev;
+
+	/* Specific data for at8031 PHYs */
+	void *data;
 };
 
 struct at803x_context {
@@ -469,27 +475,11 @@  static int at803x_set_wol(struct phy_device *phydev,
 			phy_write_mmd(phydev, MDIO_MMD_PCS, offsets[i],
 				      mac[(i * 2) + 1] | (mac[(i * 2)] << 8));
 
-		/* Enable WOL function for 1588 */
-		if (phydev->drv->phy_id == ATH8031_PHY_ID) {
-			ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
-					     AT803X_PHY_MMD3_WOL_CTRL,
-					     0, AT803X_WOL_EN);
-			if (ret)
-				return ret;
-		}
 		/* Enable WOL interrupt */
 		ret = phy_modify(phydev, AT803X_INTR_ENABLE, 0, AT803X_INTR_ENABLE_WOL);
 		if (ret)
 			return ret;
 	} else {
-		/* Disable WoL function for 1588 */
-		if (phydev->drv->phy_id == ATH8031_PHY_ID) {
-			ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
-					     AT803X_PHY_MMD3_WOL_CTRL,
-					     AT803X_WOL_EN, 0);
-			if (ret)
-				return ret;
-		}
 		/* Disable WOL interrupt */
 		ret = phy_modify(phydev, AT803X_INTR_ENABLE, AT803X_INTR_ENABLE_WOL, 0);
 		if (ret)
@@ -602,139 +592,6 @@  static int at803x_resume(struct phy_device *phydev)
 	return phy_modify(phydev, MII_BMCR, BMCR_PDOWN | BMCR_ISOLATE, 0);
 }
 
-static int at803x_rgmii_reg_set_voltage_sel(struct regulator_dev *rdev,
-					    unsigned int selector)
-{
-	struct phy_device *phydev = rdev_get_drvdata(rdev);
-
-	if (selector)
-		return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_1F,
-					     0, AT803X_DEBUG_RGMII_1V8);
-	else
-		return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_1F,
-					     AT803X_DEBUG_RGMII_1V8, 0);
-}
-
-static int at803x_rgmii_reg_get_voltage_sel(struct regulator_dev *rdev)
-{
-	struct phy_device *phydev = rdev_get_drvdata(rdev);
-	int val;
-
-	val = at803x_debug_reg_read(phydev, AT803X_DEBUG_REG_1F);
-	if (val < 0)
-		return val;
-
-	return (val & AT803X_DEBUG_RGMII_1V8) ? 1 : 0;
-}
-
-static const struct regulator_ops vddio_regulator_ops = {
-	.list_voltage = regulator_list_voltage_table,
-	.set_voltage_sel = at803x_rgmii_reg_set_voltage_sel,
-	.get_voltage_sel = at803x_rgmii_reg_get_voltage_sel,
-};
-
-static const unsigned int vddio_voltage_table[] = {
-	1500000,
-	1800000,
-};
-
-static const struct regulator_desc vddio_desc = {
-	.name = "vddio",
-	.of_match = of_match_ptr("vddio-regulator"),
-	.n_voltages = ARRAY_SIZE(vddio_voltage_table),
-	.volt_table = vddio_voltage_table,
-	.ops = &vddio_regulator_ops,
-	.type = REGULATOR_VOLTAGE,
-	.owner = THIS_MODULE,
-};
-
-static const struct regulator_ops vddh_regulator_ops = {
-};
-
-static const struct regulator_desc vddh_desc = {
-	.name = "vddh",
-	.of_match = of_match_ptr("vddh-regulator"),
-	.n_voltages = 1,
-	.fixed_uV = 2500000,
-	.ops = &vddh_regulator_ops,
-	.type = REGULATOR_VOLTAGE,
-	.owner = THIS_MODULE,
-};
-
-static int at8031_register_regulators(struct phy_device *phydev)
-{
-	struct at803x_priv *priv = phydev->priv;
-	struct device *dev = &phydev->mdio.dev;
-	struct regulator_config config = { };
-
-	config.dev = dev;
-	config.driver_data = phydev;
-
-	priv->vddio_rdev = devm_regulator_register(dev, &vddio_desc, &config);
-	if (IS_ERR(priv->vddio_rdev)) {
-		phydev_err(phydev, "failed to register VDDIO regulator\n");
-		return PTR_ERR(priv->vddio_rdev);
-	}
-
-	priv->vddh_rdev = devm_regulator_register(dev, &vddh_desc, &config);
-	if (IS_ERR(priv->vddh_rdev)) {
-		phydev_err(phydev, "failed to register VDDH regulator\n");
-		return PTR_ERR(priv->vddh_rdev);
-	}
-
-	return 0;
-}
-
-static int at803x_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
-{
-	struct phy_device *phydev = upstream;
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(phy_support);
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
-	DECLARE_PHY_INTERFACE_MASK(interfaces);
-	phy_interface_t iface;
-
-	linkmode_zero(phy_support);
-	phylink_set(phy_support, 1000baseX_Full);
-	phylink_set(phy_support, 1000baseT_Full);
-	phylink_set(phy_support, Autoneg);
-	phylink_set(phy_support, Pause);
-	phylink_set(phy_support, Asym_Pause);
-
-	linkmode_zero(sfp_support);
-	sfp_parse_support(phydev->sfp_bus, id, sfp_support, interfaces);
-	/* Some modules support 10G modes as well as others we support.
-	 * Mask out non-supported modes so the correct interface is picked.
-	 */
-	linkmode_and(sfp_support, phy_support, sfp_support);
-
-	if (linkmode_empty(sfp_support)) {
-		dev_err(&phydev->mdio.dev, "incompatible SFP module inserted\n");
-		return -EINVAL;
-	}
-
-	iface = sfp_select_interface(phydev->sfp_bus, sfp_support);
-
-	/* Only 1000Base-X is supported by AR8031/8033 as the downstream SerDes
-	 * interface for use with SFP modules.
-	 * However, some copper modules detected as having a preferred SGMII
-	 * interface do default to and function in 1000Base-X mode, so just
-	 * print a warning and allow such modules, as they may have some chance
-	 * of working.
-	 */
-	if (iface == PHY_INTERFACE_MODE_SGMII)
-		dev_warn(&phydev->mdio.dev, "module may not function if 1000Base-X not supported\n");
-	else if (iface != PHY_INTERFACE_MODE_1000BASEX)
-		return -EINVAL;
-
-	return 0;
-}
-
-static const struct sfp_upstream_ops at803x_sfp_ops = {
-	.attach = phy_sfp_attach,
-	.detach = phy_sfp_detach,
-	.module_insert = at803x_sfp_insert,
-};
-
 static int at803x_parse_dt(struct phy_device *phydev)
 {
 	struct device_node *node = phydev->mdio.dev.of_node;
@@ -828,30 +685,6 @@  static int at803x_parse_dt(struct phy_device *phydev)
 		}
 	}
 
-	/* Only supported on AR8031/AR8033, the AR8030/AR8035 use strapping
-	 * options.
-	 */
-	if (phydev->drv->phy_id == ATH8031_PHY_ID) {
-		if (of_property_read_bool(node, "qca,keep-pll-enabled"))
-			priv->flags |= AT803X_KEEP_PLL_ENABLED;
-
-		ret = at8031_register_regulators(phydev);
-		if (ret < 0)
-			return ret;
-
-		ret = devm_regulator_get_enable_optional(&phydev->mdio.dev,
-							 "vddio");
-		if (ret) {
-			phydev_err(phydev, "failed to get VDDIO regulator\n");
-			return ret;
-		}
-
-		/* Only AR8031/8033 support 1000Base-X for SFP modules */
-		ret = phy_sfp_probe(phydev, &at803x_sfp_ops);
-		if (ret < 0)
-			return ret;
-	}
-
 	return 0;
 }
 
@@ -871,56 +704,6 @@  static int at803x_probe(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	if (phydev->drv->phy_id == ATH8031_PHY_ID) {
-		int ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
-		int mode_cfg;
-
-		if (ccr < 0)
-			return ccr;
-		mode_cfg = ccr & AT803X_MODE_CFG_MASK;
-
-		switch (mode_cfg) {
-		case AT803X_MODE_CFG_BX1000_RGMII_50OHM:
-		case AT803X_MODE_CFG_BX1000_RGMII_75OHM:
-			priv->is_1000basex = true;
-			fallthrough;
-		case AT803X_MODE_CFG_FX100_RGMII_50OHM:
-		case AT803X_MODE_CFG_FX100_RGMII_75OHM:
-			priv->is_fiber = true;
-			break;
-		}
-	}
-
-	return 0;
-}
-
-static int at803x_get_features(struct phy_device *phydev)
-{
-	struct at803x_priv *priv = phydev->priv;
-	int err;
-
-	err = genphy_read_abilities(phydev);
-	if (err)
-		return err;
-
-	if (phydev->drv->phy_id != ATH8031_PHY_ID)
-		return 0;
-
-	/* AR8031/AR8033 have different status registers
-	 * for copper and fiber operation. However, the
-	 * extended status register is the same for both
-	 * operation modes.
-	 *
-	 * As a result of that, ESTATUS_1000_XFULL is set
-	 * to 1 even when operating in copper TP mode.
-	 *
-	 * Remove this mode from the supported link modes
-	 * when not operating in 1000BaseX mode.
-	 */
-	if (!priv->is_1000basex)
-		linkmode_clear_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
-				   phydev->supported);
-
 	return 0;
 }
 
@@ -998,36 +781,8 @@  static int at803x_hibernation_mode_config(struct phy_device *phydev)
 
 static int at803x_config_init(struct phy_device *phydev)
 {
-	struct at803x_priv *priv = phydev->priv;
 	int ret;
 
-	if (phydev->drv->phy_id == ATH8031_PHY_ID) {
-		/* Disable WoL in 1588 register which is enabled
-		 * by default
-		 */
-		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
-				     AT803X_PHY_MMD3_WOL_CTRL,
-				     AT803X_WOL_EN, 0);
-		if (ret)
-			return ret;
-
-		/* Some bootloaders leave the fiber page selected.
-		 * Switch to the appropriate page (fiber or copper), as otherwise we
-		 * read the PHY capabilities from the wrong page.
-		 */
-		phy_lock_mdio_bus(phydev);
-		ret = at803x_write_page(phydev,
-					priv->is_fiber ? AT803X_PAGE_FIBER :
-							 AT803X_PAGE_COPPER);
-		phy_unlock_mdio_bus(phydev);
-		if (ret)
-			return ret;
-
-		ret = at8031_pll_config(phydev);
-		if (ret < 0)
-			return ret;
-	}
-
 	/* The RX and TX delay default is:
 	 *   after HW reset: RX delay enabled and TX delay disabled
 	 *   after SW reset: RX delay enabled, while TX delay retains the
@@ -1081,7 +836,6 @@  static int at803x_ack_interrupt(struct phy_device *phydev)
 
 static int at803x_config_intr(struct phy_device *phydev)
 {
-	struct at803x_priv *priv = phydev->priv;
 	int err;
 	int value;
 
@@ -1098,10 +852,6 @@  static int at803x_config_intr(struct phy_device *phydev)
 		value |= AT803X_INTR_ENABLE_DUPLEX_CHANGED;
 		value |= AT803X_INTR_ENABLE_LINK_FAIL;
 		value |= AT803X_INTR_ENABLE_LINK_SUCCESS;
-		if (priv->is_fiber) {
-			value |= AT803X_INTR_ENABLE_LINK_FAIL_BX;
-			value |= AT803X_INTR_ENABLE_LINK_SUCCESS_BX;
-		}
 
 		err = phy_write(phydev, AT803X_INTR_ENABLE, value);
 	} else {
@@ -1234,12 +984,8 @@  static int at803x_read_specific_status(struct phy_device *phydev)
 
 static int at803x_read_status(struct phy_device *phydev)
 {
-	struct at803x_priv *priv = phydev->priv;
 	int err, old_link = phydev->link;
 
-	if (priv->is_1000basex)
-		return genphy_c37_read_status(phydev);
-
 	/* Update the link, but return if there was an error */
 	err = genphy_update_link(phydev);
 	if (err)
@@ -1293,7 +1039,6 @@  static int at803x_config_mdix(struct phy_device *phydev, u8 ctrl)
 
 static int at803x_config_aneg(struct phy_device *phydev)
 {
-	struct at803x_priv *priv = phydev->priv;
 	int ret;
 
 	ret = at803x_config_mdix(phydev, phydev->mdix_ctrl);
@@ -1310,9 +1055,6 @@  static int at803x_config_aneg(struct phy_device *phydev)
 			return ret;
 	}
 
-	if (priv->is_1000basex)
-		return genphy_c37_config_aneg(phydev);
-
 	/* Do not restart auto-negotiation by setting ret to 0 defautly,
 	 * when calling __genphy_config_aneg later.
 	 */
@@ -1594,6 +1336,351 @@  static int at803x_cable_test_start(struct phy_device *phydev)
 	return 0;
 }
 
+static int at8031_rgmii_reg_set_voltage_sel(struct regulator_dev *rdev,
+					    unsigned int selector)
+{
+	struct phy_device *phydev = rdev_get_drvdata(rdev);
+
+	if (selector)
+		return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_1F,
+					     0, AT803X_DEBUG_RGMII_1V8);
+	else
+		return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_1F,
+					     AT803X_DEBUG_RGMII_1V8, 0);
+}
+
+static int at8031_rgmii_reg_get_voltage_sel(struct regulator_dev *rdev)
+{
+	struct phy_device *phydev = rdev_get_drvdata(rdev);
+	int val;
+
+	val = at803x_debug_reg_read(phydev, AT803X_DEBUG_REG_1F);
+	if (val < 0)
+		return val;
+
+	return (val & AT803X_DEBUG_RGMII_1V8) ? 1 : 0;
+}
+
+static const struct regulator_ops vddio_regulator_ops = {
+	.list_voltage = regulator_list_voltage_table,
+	.set_voltage_sel = at8031_rgmii_reg_set_voltage_sel,
+	.get_voltage_sel = at8031_rgmii_reg_get_voltage_sel,
+};
+
+static const unsigned int vddio_voltage_table[] = {
+	1500000,
+	1800000,
+};
+
+static const struct regulator_desc vddio_desc = {
+	.name = "vddio",
+	.of_match = of_match_ptr("vddio-regulator"),
+	.n_voltages = ARRAY_SIZE(vddio_voltage_table),
+	.volt_table = vddio_voltage_table,
+	.ops = &vddio_regulator_ops,
+	.type = REGULATOR_VOLTAGE,
+	.owner = THIS_MODULE,
+};
+
+static const struct regulator_ops vddh_regulator_ops = {
+};
+
+static const struct regulator_desc vddh_desc = {
+	.name = "vddh",
+	.of_match = of_match_ptr("vddh-regulator"),
+	.n_voltages = 1,
+	.fixed_uV = 2500000,
+	.ops = &vddh_regulator_ops,
+	.type = REGULATOR_VOLTAGE,
+	.owner = THIS_MODULE,
+};
+
+static int at8031_register_regulators(struct phy_device *phydev)
+{
+	struct at803x_priv *priv = phydev->priv;
+	struct device *dev = &phydev->mdio.dev;
+	struct at8031_data *data = priv->data;
+	struct regulator_config config = { };
+
+	config.dev = dev;
+	config.driver_data = phydev;
+
+	data->vddio_rdev = devm_regulator_register(dev, &vddio_desc, &config);
+	if (IS_ERR(data->vddio_rdev)) {
+		phydev_err(phydev, "failed to register VDDIO regulator\n");
+		return PTR_ERR(data->vddio_rdev);
+	}
+
+	data->vddh_rdev = devm_regulator_register(dev, &vddh_desc, &config);
+	if (IS_ERR(data->vddh_rdev)) {
+		phydev_err(phydev, "failed to register VDDH regulator\n");
+		return PTR_ERR(data->vddh_rdev);
+	}
+
+	return 0;
+}
+
+static int at8031_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
+{
+	struct phy_device *phydev = upstream;
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(phy_support);
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
+	DECLARE_PHY_INTERFACE_MASK(interfaces);
+	phy_interface_t iface;
+
+	linkmode_zero(phy_support);
+	phylink_set(phy_support, 1000baseX_Full);
+	phylink_set(phy_support, 1000baseT_Full);
+	phylink_set(phy_support, Autoneg);
+	phylink_set(phy_support, Pause);
+	phylink_set(phy_support, Asym_Pause);
+
+	linkmode_zero(sfp_support);
+	sfp_parse_support(phydev->sfp_bus, id, sfp_support, interfaces);
+	/* Some modules support 10G modes as well as others we support.
+	 * Mask out non-supported modes so the correct interface is picked.
+	 */
+	linkmode_and(sfp_support, phy_support, sfp_support);
+
+	if (linkmode_empty(sfp_support)) {
+		dev_err(&phydev->mdio.dev, "incompatible SFP module inserted\n");
+		return -EINVAL;
+	}
+
+	iface = sfp_select_interface(phydev->sfp_bus, sfp_support);
+
+	/* Only 1000Base-X is supported by AR8031/8033 as the downstream SerDes
+	 * interface for use with SFP modules.
+	 * However, some copper modules detected as having a preferred SGMII
+	 * interface do default to and function in 1000Base-X mode, so just
+	 * print a warning and allow such modules, as they may have some chance
+	 * of working.
+	 */
+	if (iface == PHY_INTERFACE_MODE_SGMII)
+		dev_warn(&phydev->mdio.dev, "module may not function if 1000Base-X not supported\n");
+	else if (iface != PHY_INTERFACE_MODE_1000BASEX)
+		return -EINVAL;
+
+	return 0;
+}
+
+static const struct sfp_upstream_ops at8031_sfp_ops = {
+	.attach = phy_sfp_attach,
+	.detach = phy_sfp_detach,
+	.module_insert = at8031_sfp_insert,
+};
+
+static int at8031_parse_dt(struct phy_device *phydev)
+{
+	struct device_node *node = phydev->mdio.dev.of_node;
+	struct at803x_priv *priv = phydev->priv;
+	int ret;
+
+	if (of_property_read_bool(node, "qca,keep-pll-enabled"))
+		priv->flags |= AT803X_KEEP_PLL_ENABLED;
+
+	ret = at8031_register_regulators(phydev);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_regulator_get_enable_optional(&phydev->mdio.dev,
+						 "vddio");
+	if (ret) {
+		phydev_err(phydev, "failed to get VDDIO regulator\n");
+		return ret;
+	}
+
+	/* Only AR8031/8033 support 1000Base-X for SFP modules */
+	return phy_sfp_probe(phydev, &at8031_sfp_ops);
+}
+
+static int at8031_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct at8031_data *data;
+	struct at803x_priv *priv;
+	int ccr, mode_cfg;
+	int ret;
+
+	ret = at803x_probe(phydev);
+	if (ret)
+		return ret;
+
+	priv = phydev->priv;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	priv->data = data;
+
+	ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
+	if (ccr < 0)
+		return ccr;
+
+	mode_cfg = FIELD_GET(AT803X_MODE_CFG_MASK, ccr);
+
+	switch (mode_cfg) {
+	case AT803X_MODE_CFG_BX1000_RGMII_50OHM:
+	case AT803X_MODE_CFG_BX1000_RGMII_75OHM:
+		data->is_1000basex = true;
+		fallthrough;
+	case AT803X_MODE_CFG_FX100_RGMII_50OHM:
+	case AT803X_MODE_CFG_FX100_RGMII_75OHM:
+		data->is_fiber = true;
+		break;
+	}
+
+	/* Only supported on AR8031/AR8033, the AR8030/AR8035 use strapping
+	 * options.
+	 */
+	return at8031_parse_dt(phydev);
+}
+
+static int at8031_config_init(struct phy_device *phydev)
+{
+	struct at803x_priv *priv = phydev->priv;
+	struct at8031_data *data = priv->data;
+	int ret;
+
+	/* Disable WoL in 1588 register which is enabled
+	 * by default
+	 */
+	ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
+			     AT803X_PHY_MMD3_WOL_CTRL,
+			     AT803X_WOL_EN, 0);
+	if (ret)
+		return ret;
+
+	/* Some bootloaders leave the fiber page selected.
+	 * Switch to the appropriate page (fiber or copper), as otherwise we
+	 * read the PHY capabilities from the wrong page.
+	 */
+	phy_lock_mdio_bus(phydev);
+	ret = at803x_write_page(phydev,
+				data->is_fiber ? AT803X_PAGE_FIBER :
+				AT803X_PAGE_COPPER);
+	phy_unlock_mdio_bus(phydev);
+	if (ret)
+		return ret;
+
+	ret = at8031_pll_config(phydev);
+	if (ret < 0)
+		return ret;
+
+	return at803x_config_init(phydev);
+}
+
+static int at8031_config_intr(struct phy_device *phydev)
+{
+	struct at803x_priv *priv = phydev->priv;
+	struct at8031_data *data = priv->data;
+	int err, value = 0;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED &&
+	    data->is_fiber) {
+		/* Clear any pending interrupts */
+		err = at803x_ack_interrupt(phydev);
+		if (err)
+			return err;
+
+		value |= AT803X_INTR_ENABLE_LINK_FAIL_BX;
+		value |= AT803X_INTR_ENABLE_LINK_SUCCESS_BX;
+
+		err = phy_set_bits(phydev, AT803X_INTR_ENABLE, value);
+		if (err)
+			return err;
+	}
+
+	return at803x_config_intr(phydev);
+}
+
+static int at8031_get_features(struct phy_device *phydev)
+{
+	struct at803x_priv *priv = phydev->priv;
+	struct at8031_data *data = priv->data;
+	int err;
+
+	err = genphy_read_abilities(phydev);
+	if (err)
+		return err;
+
+	/* AR8031/AR8033 have different status registers
+	 * for copper and fiber operation. However, the
+	 * extended status register is the same for both
+	 * operation modes.
+	 *
+	 * As a result of that, ESTATUS_1000_XFULL is set
+	 * to 1 even when operating in copper TP mode.
+	 *
+	 * Remove this mode from the supported link modes
+	 * when not operating in 1000BaseX mode.
+	 */
+	if (!data->is_1000basex)
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+				   phydev->supported);
+
+	return 0;
+}
+
+static int at8031_read_status(struct phy_device *phydev)
+{
+	struct at803x_priv *priv = phydev->priv;
+	struct at8031_data *data = priv->data;
+
+	if (data->is_1000basex)
+		return genphy_c37_read_status(phydev);
+
+	return at803x_read_status(phydev);
+}
+
+static int at8031_config_aneg(struct phy_device *phydev)
+{
+	struct at803x_priv *priv = phydev->priv;
+	struct at8031_data *data = priv->data;
+	int ret;
+
+	ret = at803x_config_mdix(phydev, phydev->mdix_ctrl);
+	if (ret < 0)
+		return ret;
+
+	/* Changes of the midx bits are disruptive to the normal operation;
+	 * therefore any changes to these registers must be followed by a
+	 * software reset to take effect.
+	 */
+	if (ret == 1) {
+		ret = genphy_soft_reset(phydev);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (data->is_1000basex)
+		return genphy_c37_config_aneg(phydev);
+
+	return __genphy_config_aneg(phydev, ret);
+}
+
+static int at8031_set_wol(struct phy_device *phydev,
+			  struct ethtool_wolinfo *wol)
+{
+	int ret;
+
+	if (wol->wolopts & WAKE_MAGIC)
+		/* Enable WOL function for 1588 */
+		ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
+				       AT803X_PHY_MMD3_WOL_CTRL,
+				       AT803X_WOL_EN);
+	else
+		/* Disable WoL function for 1588 */
+		ret = phy_clear_bits_mmd(phydev, MDIO_MMD_PCS,
+					 AT803X_PHY_MMD3_WOL_CTRL,
+					 AT803X_WOL_EN);
+	if (ret)
+		return ret;
+
+	return at803x_set_wol(phydev, wol);
+}
+
 static int qca83xx_probe(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;
@@ -2116,19 +2203,19 @@  static struct phy_driver at803x_driver[] = {
 	PHY_ID_MATCH_EXACT(ATH8031_PHY_ID),
 	.name			= "Qualcomm Atheros AR8031/AR8033",
 	.flags			= PHY_POLL_CABLE_TEST,
-	.probe			= at803x_probe,
-	.config_init		= at803x_config_init,
-	.config_aneg		= at803x_config_aneg,
+	.probe			= at8031_probe,
+	.config_init		= at8031_config_init,
+	.config_aneg		= at8031_config_aneg,
 	.soft_reset		= genphy_soft_reset,
-	.set_wol		= at803x_set_wol,
+	.set_wol		= at8031_set_wol,
 	.get_wol		= at803x_get_wol,
 	.suspend		= at803x_suspend,
 	.resume			= at803x_resume,
 	.read_page		= at803x_read_page,
 	.write_page		= at803x_write_page,
-	.get_features		= at803x_get_features,
-	.read_status		= at803x_read_status,
-	.config_intr		= at803x_config_intr,
+	.get_features		= at8031_get_features,
+	.read_status		= at8031_read_status,
+	.config_intr		= at8031_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
 	.get_tunable		= at803x_get_tunable,
 	.set_tunable		= at803x_set_tunable,