[net-next PATCH 00/14] net: phy: at803x: cleanup + split

[net-next PATCH 00/14] net: phy: at803x: cleanup + split

[Christian Marangi]

The intention of this big series is to try to cleanup and split
the big at803x PHY driver.

It currently have 3 different family of PHY in it. at803x, qca83xx
and qca808x.

The current codebase required lots of cleanup and reworking to
make the split possible as currently there is a greater use of
adding special function matching the phy_id.

This has been reworked to make the function actually generic
and make the change only in more specific one. The result
is the addition of micro additional function but that is for good
as it massively simplify splitting the driver later.

The main bonus of this cleanup is creating smaller PHY drivers
since they won't have all the bloat of unused functions or
extra condition (especially all the stuff related to regulators
that are only handled by the at8031 PHY)

Consider that this is all in preparation for the addition of
qca807x PHY driver that will also uso some of the functions of
at803x.

New Kconfig are introduced for the split PHY driver as they are
now built as separate PHY drivers.

Christian Marangi (14):
  net: phy: at803x: fix passing the wrong reference for config_intr
  net: phy: at803x: move disable WOL for 8031 from probe to config
  net: phy: at803x: raname hw_stats functions to qca83xx specific name
  net: phy: at803x: move qca83xx stats out of generic at803x_priv struct
  net: phy: at803x: move qca83xx specific check in dedicated functions
  net: phy: at803x: move at8031 specific data out of generic at803x_priv
  net: phy: at803x: move at8035 specific DT parse to dedicated probe
  net: phy: at803x: drop specific PHY id check from cable test functions
  net: phy: at803x: remove specific qca808x check from at803x functions
  net: phy: at803x: drop usless probe for qca8081 PHY
  net: phy: at803x: make specific status mask more generic
  net: phy: move at803x PHY driver to dedicated directory
  net: phy: qcom: deatch qca83xx PHY driver from at803x
  net: phy: qcom: detach qca808x PHY driver from at803x

 drivers/net/phy/Kconfig        |    7 +-
 drivers/net/phy/Makefile       |    2 +-
 drivers/net/phy/at803x.c       | 2248 --------------------------------
 drivers/net/phy/qcom/Kconfig   |   17 +
 drivers/net/phy/qcom/Makefile  |    4 +
 drivers/net/phy/qcom/at803x.c  | 1222 +++++++++++++++++
 drivers/net/phy/qcom/common.c  |  351 +++++
 drivers/net/phy/qcom/qca808x.c |  550 ++++++++
 drivers/net/phy/qcom/qca83xx.c |  275 ++++
 drivers/net/phy/qcom/qcom.h    |  124 ++
 10 files changed, 2545 insertions(+), 2255 deletions(-)
 delete mode 100644 drivers/net/phy/at803x.c
 create mode 100644 drivers/net/phy/qcom/Kconfig
 create mode 100644 drivers/net/phy/qcom/Makefile
 create mode 100644 drivers/net/phy/qcom/at803x.c
 create mode 100644 drivers/net/phy/qcom/common.c
 create mode 100644 drivers/net/phy/qcom/qca808x.c
 create mode 100644 drivers/net/phy/qcom/qca83xx.c
 create mode 100644 drivers/net/phy/qcom/qcom.h

-- 
2.40.1

[net-next PATCH 01/14] net: phy: at803x: fix passing the wrong reference for config_intr

[Christian Marangi]

Fix passing the wrong reference for config_initr on passing the function
pointer, drop the wrong & from at803x_config_intr in the PHY struct.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/at803x.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 37fb033e1c29..ef203b0807e5 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -2104,7 +2104,7 @@ static struct phy_driver at803x_driver[] = {
 	.write_page		= at803x_write_page,
 	.get_features		= at803x_get_features,
 	.read_status		= at803x_read_status,
-	.config_intr		= &at803x_config_intr,
+	.config_intr		= at803x_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
 	.get_tunable		= at803x_get_tunable,
 	.set_tunable		= at803x_set_tunable,
@@ -2134,7 +2134,7 @@ static struct phy_driver at803x_driver[] = {
 	.resume			= at803x_resume,
 	.flags			= PHY_POLL_CABLE_TEST,
 	/* PHY_BASIC_FEATURES */
-	.config_intr		= &at803x_config_intr,
+	.config_intr		= at803x_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
 	.cable_test_start	= at803x_cable_test_start,
 	.cable_test_get_status	= at803x_cable_test_get_status,
@@ -2150,7 +2150,7 @@ static struct phy_driver at803x_driver[] = {
 	.resume			= at803x_resume,
 	.flags			= PHY_POLL_CABLE_TEST,
 	/* PHY_BASIC_FEATURES */
-	.config_intr		= &at803x_config_intr,
+	.config_intr		= at803x_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
 	.cable_test_start	= at803x_cable_test_start,
 	.cable_test_get_status	= at803x_cable_test_get_status,
-- 
2.40.1

Re: [net-next PATCH 01/14] net: phy: at803x: fix passing the wrong reference for config_intr

[Andrew Lunn]

On Wed, Nov 29, 2023 at 03:12:06AM +0100, Christian Marangi wrote:
> Fix passing the wrong reference for config_initr on passing the function
> pointer, drop the wrong & from at803x_config_intr in the PHY struct.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[net-next PATCH 02/14] net: phy: at803x: move disable WOL for 8031 from probe to config

[Christian Marangi]

Probe should be used only for DT parsing and allocate required priv, it
shouldn't touch regs, there is config_init for that.

Move the WOL disable call from probe to config_init to follow this rule
and keep code tidy.

No behaviour is done as the mode was disabled only if phy_read succeeded
in probe and this is translated as the first action done in config_init
(called only if probe returns 0)

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/at803x.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index ef203b0807e5..b32ff82240dc 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -886,15 +886,6 @@ static int at803x_probe(struct phy_device *phydev)
 			priv->is_fiber = true;
 			break;
 		}
-
-		/* 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;
 	}
 
 	return 0;
@@ -1008,6 +999,15 @@ static int at803x_config_init(struct phy_device *phydev)
 	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.
-- 
2.40.1

Re: [net-next PATCH 02/14] net: phy: at803x: move disable WOL for 8031 from probe to config

[Russell King (Oracle)]

On Wed, Nov 29, 2023 at 03:12:07AM +0100, Christian Marangi wrote:
> Probe should be used only for DT parsing and allocate required priv, it
> shouldn't touch regs, there is config_init for that.

I'm not sure where you get that idea from. PHY driver probe() functions
are permitted to access registers to do any setup that they wish to.

config_init() is to configure the PHY for use with the network
interface.

I think this patch is just noise rather than a cleanup.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [net-next PATCH 02/14] net: phy: at803x: move disable WOL for 8031 from probe to config

[Christian Marangi]

On Wed, Nov 29, 2023 at 09:24:32AM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 29, 2023 at 03:12:07AM +0100, Christian Marangi wrote:
> > Probe should be used only for DT parsing and allocate required priv, it
> > shouldn't touch regs, there is config_init for that.
> 
> I'm not sure where you get that idea from. PHY driver probe() functions
> are permitted to access registers to do any setup that they wish to.
> 
> config_init() is to configure the PHY for use with the network
> interface.
> 
> I think this patch is just noise rather than a cleanup.
>

I got it from here [1]

Also on every other driver probe was always used for allocation and
parsing so why deviates from this pattern here?

Also I think it was wrong from the start as on reset I think WoL is
not disabled again. (probe is not called)

[1] https://elixir.bootlin.com/linux/latest/source/include/linux/phy.h#L916

-- 
	Ansuel

Re: [net-next PATCH 02/14] net: phy: at803x: move disable WOL for 8031 from probe to config

[Russell King (Oracle)]

On Wed, Nov 29, 2023 at 10:36:31AM +0100, Christian Marangi wrote:
> On Wed, Nov 29, 2023 at 09:24:32AM +0000, Russell King (Oracle) wrote:
> > On Wed, Nov 29, 2023 at 03:12:07AM +0100, Christian Marangi wrote:
> > > Probe should be used only for DT parsing and allocate required priv, it
> > > shouldn't touch regs, there is config_init for that.
> > 
> > I'm not sure where you get that idea from. PHY driver probe() functions
> > are permitted to access registers to do any setup that they wish to.
> > 
> > config_init() is to configure the PHY for use with the network
> > interface.
> > 
> > I think this patch is just noise rather than a cleanup.
> >
> 
> I got it from here [1]
> 
> Also on every other driver probe was always used for allocation and
> parsing so why deviates from this pattern here?

Untrue.

bcm54140_enable_monitoring() is called from bcm54140_probe_once()
which in turn is called from bcm54140_probe().

dp83869_probe() calls dp83869_config_init(), rightly or wrongly.

lxt973_probe() fixes up the BMCR.

mv3310_probe() configures power-down modes, modifying registers.

mt7988_phy_probe() calls mt7988_phy_fix_leds_polarities() which
modifies registers.

lan8814_probe() calls lan8814_ptp_init() which does a whole load of
register writes.

lan88xx_probe() configures LEDs via register writes.

yt8521_probe() configures clocks via register modification.

I'm afraid this means your comment is demonstrably false.

> Also I think it was wrong from the start as on reset I think WoL is
> not disabled again. (probe is not called)

On hardware reset, the 1588 register will re-enable the WoL pin, but
that needs a hardware reset of the PHY to happen after probe() is
called.

However, phy_probe() will only assert the reset signal _if_ an error
occured during probing, not if probing was successful. So, a successful
probe of this driver will not cause a hardware reset.

Also, hardware reset is optional. Do you know whether the platforms
that use the separate WoL pin which this 1588 register controls also
wire the reset signal such that it can be controlled by Linux?
Probably not.

So, this register write will not be cleared by a hardware reset after
a successful probe.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [net-next PATCH 02/14] net: phy: at803x: move disable WOL for 8031 from probe to config

[Christian Marangi]

On Wed, Nov 29, 2023 at 10:45:11AM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 29, 2023 at 10:36:31AM +0100, Christian Marangi wrote:
> > On Wed, Nov 29, 2023 at 09:24:32AM +0000, Russell King (Oracle) wrote:
> > > On Wed, Nov 29, 2023 at 03:12:07AM +0100, Christian Marangi wrote:
> > > > Probe should be used only for DT parsing and allocate required priv, it
> > > > shouldn't touch regs, there is config_init for that.
> > > 
> > > I'm not sure where you get that idea from. PHY driver probe() functions
> > > are permitted to access registers to do any setup that they wish to.
> > > 
> > > config_init() is to configure the PHY for use with the network
> > > interface.
> > > 
> > > I think this patch is just noise rather than a cleanup.
> > >
> > 
> > I got it from here [1]
> > 
> > Also on every other driver probe was always used for allocation and
> > parsing so why deviates from this pattern here?
> 
> Untrue.
> 
> bcm54140_enable_monitoring() is called from bcm54140_probe_once()
> which in turn is called from bcm54140_probe().
> 
> dp83869_probe() calls dp83869_config_init(), rightly or wrongly.
> 
> lxt973_probe() fixes up the BMCR.
> 
> mv3310_probe() configures power-down modes, modifying registers.
> 
> mt7988_phy_probe() calls mt7988_phy_fix_leds_polarities() which
> modifies registers.
> 
> lan8814_probe() calls lan8814_ptp_init() which does a whole load of
> register writes.
> 
> lan88xx_probe() configures LEDs via register writes.
> 
> yt8521_probe() configures clocks via register modification.
> 
> I'm afraid this means your comment is demonstrably false.
>

Don't want to start a discussion and thanks a lot for pointing this
out. This is really to talk about this and not contradict you.

Yes it's not true, and I was wrong but still I watch other driver
outside PHY and normally probe should not do that kind of stuff.
(even the comments describing the use in phy.h doesn't say that it
should be used for configuring the PHY only once on discovery)

Watching some of the thing, I feel all of this is done in probe as it's
not called again on software reset (as it will call config_init again).

This looks like there is a missing feature here and maybe a chance to
improve this?

Wonder if adding an additional OP would be beneficial to this?
Was thinking to something like

.config_once ?

And add some comments that it's called only on PHY discovery?
It would be put right after the probe call in phy_core.

> > Also I think it was wrong from the start as on reset I think WoL is
> > not disabled again. (probe is not called)
> 
> On hardware reset, the 1588 register will re-enable the WoL pin, but
> that needs a hardware reset of the PHY to happen after probe() is
> called.
> 
> However, phy_probe() will only assert the reset signal _if_ an error
> occured during probing, not if probing was successful. So, a successful
> probe of this driver will not cause a hardware reset.
> 
> Also, hardware reset is optional. Do you know whether the platforms
> that use the separate WoL pin which this 1588 register controls also
> wire the reset signal such that it can be controlled by Linux?
> Probably not.
> 
> So, this register write will not be cleared by a hardware reset after
> a successful probe.
>

I just checked Datasheet, on HW reset it's enabled again and on SW reset
value is retained. So yes it must stay in probe as it will cause side
effect if WoL get enabled after... Really hope the config_once thing is
OK since it will make things much cleaner.

-- 
	Ansuel

Re: [net-next PATCH 02/14] net: phy: at803x: move disable WOL for 8031 from probe to config

[Russell King (Oracle)]

On Wed, Nov 29, 2023 at 12:03:14PM +0100, Christian Marangi wrote:
> On Wed, Nov 29, 2023 at 10:45:11AM +0000, Russell King (Oracle) wrote:
> > On Wed, Nov 29, 2023 at 10:36:31AM +0100, Christian Marangi wrote:
> > > On Wed, Nov 29, 2023 at 09:24:32AM +0000, Russell King (Oracle) wrote:
> > > > On Wed, Nov 29, 2023 at 03:12:07AM +0100, Christian Marangi wrote:
> > > > > Probe should be used only for DT parsing and allocate required priv, it
> > > > > shouldn't touch regs, there is config_init for that.
> > > > 
> > > > I'm not sure where you get that idea from. PHY driver probe() functions
> > > > are permitted to access registers to do any setup that they wish to.
> > > > 
> > > > config_init() is to configure the PHY for use with the network
> > > > interface.
> > > > 
> > > > I think this patch is just noise rather than a cleanup.
> > > >
> > > 
> > > I got it from here [1]
> > > 
> > > Also on every other driver probe was always used for allocation and
> > > parsing so why deviates from this pattern here?
> > 
> > Untrue.
> > 
> > bcm54140_enable_monitoring() is called from bcm54140_probe_once()
> > which in turn is called from bcm54140_probe().
> > 
> > dp83869_probe() calls dp83869_config_init(), rightly or wrongly.
> > 
> > lxt973_probe() fixes up the BMCR.
> > 
> > mv3310_probe() configures power-down modes, modifying registers.
> > 
> > mt7988_phy_probe() calls mt7988_phy_fix_leds_polarities() which
> > modifies registers.
> > 
> > lan8814_probe() calls lan8814_ptp_init() which does a whole load of
> > register writes.
> > 
> > lan88xx_probe() configures LEDs via register writes.
> > 
> > yt8521_probe() configures clocks via register modification.
> > 
> > I'm afraid this means your comment is demonstrably false.
> >
> 
> Don't want to start a discussion and thanks a lot for pointing this
> out. This is really to talk about this and not contradict you.
> 
> Yes it's not true, and I was wrong but still I watch other driver
> outside PHY and normally probe should not do that kind of stuff.
> (even the comments describing the use in phy.h doesn't say that it
> should be used for configuring the PHY only once on discovery)
> 
> Watching some of the thing, I feel all of this is done in probe as it's
> not called again on software reset (as it will call config_init again).
> 
> This looks like there is a missing feature here and maybe a chance to
> improve this?
> 
> Wonder if adding an additional OP would be beneficial to this?
> Was thinking to something like
> 
> .config_once ?
> 
> And add some comments that it's called only on PHY discovery?
> It would be put right after the probe call in phy_core.
> 
> > > Also I think it was wrong from the start as on reset I think WoL is
> > > not disabled again. (probe is not called)
> > 
> > On hardware reset, the 1588 register will re-enable the WoL pin, but
> > that needs a hardware reset of the PHY to happen after probe() is
> > called.
> > 
> > However, phy_probe() will only assert the reset signal _if_ an error
> > occured during probing, not if probing was successful. So, a successful
> > probe of this driver will not cause a hardware reset.
> > 
> > Also, hardware reset is optional. Do you know whether the platforms
> > that use the separate WoL pin which this 1588 register controls also
> > wire the reset signal such that it can be controlled by Linux?
> > Probably not.
> > 
> > So, this register write will not be cleared by a hardware reset after
> > a successful probe.
> >
> 
> I just checked Datasheet, on HW reset it's enabled again and on SW reset
> value is retained. So yes it must stay in probe as it will cause side
> effect if WoL get enabled after... Really hope the config_once thing is
> OK since it will make things much cleaner.

The phylib comment deviates from standard practice elsewhere in the
kernel, where it is totally fine to do hardware setup in the probe()
function. So I would suggest just updating the comment on probe() to
remove the idea that one shouldn't be doing this. I think it's totally
fine to be doing setup in probe().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [net-next PATCH 02/14] net: phy: at803x: move disable WOL for 8031 from probe to config

[Andrew Lunn]

>  	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;
> +

Maybe it comes later in the patch series, but i would actually add a
at8031_probe() which calls the common at803x_probe() and then does
this WoL stuff.

I don't see any reason to have just one probe, with

	if (phydev->drv->phy_id == ATH8031_PHY_ID) {

in it.

   Andrew

[net-next PATCH 03/14] net: phy: at803x: raname hw_stats functions to qca83xx specific name

[Christian Marangi]

The function and the struct related to hw_stats were specific to qca83xx
PHY but were called following the convention in the driver of calling
everything with at803x prefix.

To better organize the code, rename these function a more specific name
to better describe that they are specific to 83xx PHY family.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/at803x.c | 44 ++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index b32ff82240dc..4ff41d70fc47 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -295,7 +295,7 @@ struct at803x_hw_stat {
 	enum stat_access_type access_type;
 };
 
-static struct at803x_hw_stat at803x_hw_stats[] = {
+static struct at803x_hw_stat qca83xx_hw_stats[] = {
 	{ "phy_idle_errors", 0xa, GENMASK(7, 0), PHY},
 	{ "phy_receive_errors", 0x15, GENMASK(15, 0), PHY},
 	{ "eee_wake_errors", 0x16, GENMASK(15, 0), MMD},
@@ -311,7 +311,7 @@ struct at803x_priv {
 	bool is_1000basex;
 	struct regulator_dev *vddio_rdev;
 	struct regulator_dev *vddh_rdev;
-	u64 stats[ARRAY_SIZE(at803x_hw_stats)];
+	u64 stats[ARRAY_SIZE(qca83xx_hw_stats)];
 };
 
 struct at803x_context {
@@ -529,24 +529,24 @@ static void at803x_get_wol(struct phy_device *phydev,
 		wol->wolopts |= WAKE_MAGIC;
 }
 
-static int at803x_get_sset_count(struct phy_device *phydev)
+static int qca83xx_get_sset_count(struct phy_device *phydev)
 {
-	return ARRAY_SIZE(at803x_hw_stats);
+	return ARRAY_SIZE(qca83xx_hw_stats);
 }
 
-static void at803x_get_strings(struct phy_device *phydev, u8 *data)
+static void qca83xx_get_strings(struct phy_device *phydev, u8 *data)
 {
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(at803x_hw_stats); i++) {
+	for (i = 0; i < ARRAY_SIZE(qca83xx_hw_stats); i++) {
 		strscpy(data + i * ETH_GSTRING_LEN,
-			at803x_hw_stats[i].string, ETH_GSTRING_LEN);
+			qca83xx_hw_stats[i].string, ETH_GSTRING_LEN);
 	}
 }
 
-static u64 at803x_get_stat(struct phy_device *phydev, int i)
+static u64 qca83xx_get_stat(struct phy_device *phydev, int i)
 {
-	struct at803x_hw_stat stat = at803x_hw_stats[i];
+	struct at803x_hw_stat stat = qca83xx_hw_stats[i];
 	struct at803x_priv *priv = phydev->priv;
 	int val;
 	u64 ret;
@@ -567,13 +567,13 @@ static u64 at803x_get_stat(struct phy_device *phydev, int i)
 	return ret;
 }
 
-static void at803x_get_stats(struct phy_device *phydev,
-			     struct ethtool_stats *stats, u64 *data)
+static void qca83xx_get_stats(struct phy_device *phydev,
+			      struct ethtool_stats *stats, u64 *data)
 {
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(at803x_hw_stats); i++)
-		data[i] = at803x_get_stat(phydev, i);
+	for (i = 0; i < ARRAY_SIZE(qca83xx_hw_stats); i++)
+		data[i] = qca83xx_get_stat(phydev, i);
 }
 
 static int at803x_suspend(struct phy_device *phydev)
@@ -2168,9 +2168,9 @@ static struct phy_driver at803x_driver[] = {
 	.flags			= PHY_IS_INTERNAL,
 	.config_init		= qca83xx_config_init,
 	.soft_reset		= genphy_soft_reset,
-	.get_sset_count		= at803x_get_sset_count,
-	.get_strings		= at803x_get_strings,
-	.get_stats		= at803x_get_stats,
+	.get_sset_count		= qca83xx_get_sset_count,
+	.get_strings		= qca83xx_get_strings,
+	.get_stats		= qca83xx_get_stats,
 	.suspend		= qca83xx_suspend,
 	.resume			= qca83xx_resume,
 }, {
@@ -2184,9 +2184,9 @@ static struct phy_driver at803x_driver[] = {
 	.flags			= PHY_IS_INTERNAL,
 	.config_init		= qca83xx_config_init,
 	.soft_reset		= genphy_soft_reset,
-	.get_sset_count		= at803x_get_sset_count,
-	.get_strings		= at803x_get_strings,
-	.get_stats		= at803x_get_stats,
+	.get_sset_count		= qca83xx_get_sset_count,
+	.get_strings		= qca83xx_get_strings,
+	.get_stats		= qca83xx_get_stats,
 	.suspend		= qca83xx_suspend,
 	.resume			= qca83xx_resume,
 }, {
@@ -2200,9 +2200,9 @@ static struct phy_driver at803x_driver[] = {
 	.flags			= PHY_IS_INTERNAL,
 	.config_init		= qca83xx_config_init,
 	.soft_reset		= genphy_soft_reset,
-	.get_sset_count		= at803x_get_sset_count,
-	.get_strings		= at803x_get_strings,
-	.get_stats		= at803x_get_stats,
+	.get_sset_count		= qca83xx_get_sset_count,
+	.get_strings		= qca83xx_get_strings,
+	.get_stats		= qca83xx_get_stats,
 	.suspend		= qca83xx_suspend,
 	.resume			= qca83xx_resume,
 }, {
-- 
2.40.1

Re: [net-next PATCH 03/14] net: phy: at803x: raname hw_stats functions to qca83xx specific name

[Andrew Lunn]

On Wed, Nov 29, 2023 at 03:12:08AM +0100, Christian Marangi wrote:
> The function and the struct related to hw_stats were specific to qca83xx
> PHY but were called following the convention in the driver of calling
> everything with at803x prefix.
> 
> To better organize the code, rename these function a more specific name
> to better describe that they are specific to 83xx PHY family.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[net-next PATCH 04/14] net: phy: at803x: move qca83xx stats out of generic at803x_priv struct

[Christian Marangi]

Introduce a specific priv struct for qca83xx PHYs to store hw stats
data and a specific probe to allocate this alternative priv struct.

This also have the benefits of reducing memory allocated for every other
at803x PHY since only qca83xx currently supports storing hw stats.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/at803x.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 4ff41d70fc47..3b7baa4bb637 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -301,6 +301,10 @@ static struct at803x_hw_stat qca83xx_hw_stats[] = {
 	{ "eee_wake_errors", 0x16, GENMASK(15, 0), MMD},
 };
 
+struct qca83xx_priv {
+	u64 stats[ARRAY_SIZE(qca83xx_hw_stats)];
+};
+
 struct at803x_priv {
 	int flags;
 	u16 clk_25m_reg;
@@ -311,7 +315,6 @@ struct at803x_priv {
 	bool is_1000basex;
 	struct regulator_dev *vddio_rdev;
 	struct regulator_dev *vddh_rdev;
-	u64 stats[ARRAY_SIZE(qca83xx_hw_stats)];
 };
 
 struct at803x_context {
@@ -547,7 +550,7 @@ static void qca83xx_get_strings(struct phy_device *phydev, u8 *data)
 static u64 qca83xx_get_stat(struct phy_device *phydev, int i)
 {
 	struct at803x_hw_stat stat = qca83xx_hw_stats[i];
-	struct at803x_priv *priv = phydev->priv;
+	struct qca83xx_priv *priv = phydev->priv;
 	int val;
 	u64 ret;
 
@@ -1591,6 +1594,20 @@ static int at803x_cable_test_start(struct phy_device *phydev)
 	return 0;
 }
 
+static int qca83xx_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct qca83xx_priv *priv;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	phydev->priv = priv;
+
+	return 0;
+}
+
 static int qca83xx_config_init(struct phy_device *phydev)
 {
 	u8 switch_revision;
@@ -2164,7 +2181,7 @@ static struct phy_driver at803x_driver[] = {
 	.name			= "Qualcomm Atheros 8337 internal PHY",
 	/* PHY_GBIT_FEATURES */
 	.link_change_notify	= qca83xx_link_change_notify,
-	.probe			= at803x_probe,
+	.probe			= qca83xx_probe,
 	.flags			= PHY_IS_INTERNAL,
 	.config_init		= qca83xx_config_init,
 	.soft_reset		= genphy_soft_reset,
@@ -2180,7 +2197,7 @@ static struct phy_driver at803x_driver[] = {
 	.name			= "Qualcomm Atheros 8327-A internal PHY",
 	/* PHY_GBIT_FEATURES */
 	.link_change_notify	= qca83xx_link_change_notify,
-	.probe			= at803x_probe,
+	.probe			= qca83xx_probe,
 	.flags			= PHY_IS_INTERNAL,
 	.config_init		= qca83xx_config_init,
 	.soft_reset		= genphy_soft_reset,
@@ -2196,7 +2213,7 @@ static struct phy_driver at803x_driver[] = {
 	.name			= "Qualcomm Atheros 8327-B internal PHY",
 	/* PHY_GBIT_FEATURES */
 	.link_change_notify	= qca83xx_link_change_notify,
-	.probe			= at803x_probe,
+	.probe			= qca83xx_probe,
 	.flags			= PHY_IS_INTERNAL,
 	.config_init		= qca83xx_config_init,
 	.soft_reset		= genphy_soft_reset,
-- 
2.40.1

Re: [net-next PATCH 04/14] net: phy: at803x: move qca83xx stats out of generic at803x_priv struct

[Russell King (Oracle)]

On Wed, Nov 29, 2023 at 03:12:09AM +0100, Christian Marangi wrote:
> +struct qca83xx_priv {
> +	u64 stats[ARRAY_SIZE(qca83xx_hw_stats)];
> +};

If QCA83xx is going to use an entirely separate private data structure,
then it's clearly a separate driver, and it should be separated from
this driver. Having two incompatible private data structures in
phydev->priv in the same driver is a recipe for future errors, where
functions that expect one private data structure may be called when
the other private data structure is stored in phydev->priv.

So, if we're going to do this, then the QCA83xx support needs to
_first_ be split from this driver.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [net-next PATCH 04/14] net: phy: at803x: move qca83xx stats out of generic at803x_priv struct

[Christian Marangi]

On Wed, Nov 29, 2023 at 09:29:24AM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 29, 2023 at 03:12:09AM +0100, Christian Marangi wrote:
> > +struct qca83xx_priv {
> > +	u64 stats[ARRAY_SIZE(qca83xx_hw_stats)];
> > +};
> 
> If QCA83xx is going to use an entirely separate private data structure,
> then it's clearly a separate driver, and it should be separated from
> this driver. Having two incompatible private data structures in
> phydev->priv in the same driver is a recipe for future errors, where
> functions that expect one private data structure may be called when
> the other private data structure is stored in phydev->priv.
> 
> So, if we're going to do this, then the QCA83xx support needs to
> _first_ be split from this driver.
> 

As you notice later, it's really to make the split easier by first
separating all the functions and then moving the function in the
separate files.

Idea was to only move them, ok to make the probe and this change when
the PHY driver is detached but I feel it would make even more changes in
that patch. (Instead of simply removing things from 803x.c)

-- 
	Ansuel

Re: [net-next PATCH 04/14] net: phy: at803x: move qca83xx stats out of generic at803x_priv struct

[Andrew Lunn]

On Wed, Nov 29, 2023 at 03:12:09AM +0100, Christian Marangi wrote:
> Introduce a specific priv struct for qca83xx PHYs to store hw stats
> data and a specific probe to allocate this alternative priv struct.
> 
> This also have the benefits of reducing memory allocated for every other
> at803x PHY since only qca83xx currently supports storing hw stats.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/phy/at803x.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 4ff41d70fc47..3b7baa4bb637 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -301,6 +301,10 @@ static struct at803x_hw_stat qca83xx_hw_stats[] = {
>  	{ "eee_wake_errors", 0x16, GENMASK(15, 0), MMD},
>  };
>  
> +struct qca83xx_priv {
> +	u64 stats[ARRAY_SIZE(qca83xx_hw_stats)];
> +};
> +
>  struct at803x_priv {
>  	int flags;
>  	u16 clk_25m_reg;
> @@ -311,7 +315,6 @@ struct at803x_priv {
>  	bool is_1000basex;
>  	struct regulator_dev *vddio_rdev;
>  	struct regulator_dev *vddh_rdev;
> -	u64 stats[ARRAY_SIZE(qca83xx_hw_stats)];
>  };

I agree with Russell here, this is the wrong way to go.

Maybe keep at803x_priv for all the common private members which are
shared by all variants. Add a qca83xx_priv which includes this:

struct qca83xx_priv {
	struct at803x_priv at803_priv;
	u64 stats[ARRAY_SIZE(qca83xx_hw_stats)];
};

	Andrew

[net-next PATCH 05/14] net: phy: at803x: move qca83xx specific check in dedicated functions

[Christian Marangi]

Rework qca83xx specific check to dedicated function to tidy things up
and drop useless phy_id check.

Also drop an useless link_change_notify for QCA8337 as it did nothing an
returned early.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/at803x.c | 68 ++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 31 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 3b7baa4bb637..9a590124d1fe 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -1633,27 +1633,26 @@ static int qca83xx_config_init(struct phy_device *phydev)
 		break;
 	}
 
+	/* Following original QCA sourcecode set port to prefer master */
+	phy_set_bits(phydev, MII_CTRL1000, CTL1000_PREFER_MASTER);
+
+	return 0;
+}
+
+static int qca8327_config_init(struct phy_device *phydev)
+{
 	/* QCA8327 require DAC amplitude adjustment for 100m set to +6%.
 	 * Disable on init and enable only with 100m speed following
 	 * qca original source code.
 	 */
-	if (phydev->drv->phy_id == QCA8327_A_PHY_ID ||
-	    phydev->drv->phy_id == QCA8327_B_PHY_ID)
-		at803x_debug_reg_mask(phydev, AT803X_DEBUG_ANALOG_TEST_CTRL,
-				      QCA8327_DEBUG_MANU_CTRL_EN, 0);
+	at803x_debug_reg_mask(phydev, AT803X_DEBUG_ANALOG_TEST_CTRL,
+			      QCA8327_DEBUG_MANU_CTRL_EN, 0);
 
-	/* Following original QCA sourcecode set port to prefer master */
-	phy_set_bits(phydev, MII_CTRL1000, CTL1000_PREFER_MASTER);
-
-	return 0;
+	return qca83xx_config_init(phydev);
 }
 
 static void qca83xx_link_change_notify(struct phy_device *phydev)
 {
-	/* QCA8337 doesn't require DAC Amplitude adjustement */
-	if (phydev->drv->phy_id == QCA8337_PHY_ID)
-		return;
-
 	/* Set DAC Amplitude adjustment to +6% for 100m on link running */
 	if (phydev->state == PHY_RUNNING) {
 		if (phydev->speed == SPEED_100)
@@ -1696,19 +1695,6 @@ static int qca83xx_resume(struct phy_device *phydev)
 
 static int qca83xx_suspend(struct phy_device *phydev)
 {
-	u16 mask = 0;
-
-	/* Only QCA8337 support actual suspend.
-	 * QCA8327 cause port unreliability when phy suspend
-	 * is set.
-	 */
-	if (phydev->drv->phy_id == QCA8337_PHY_ID) {
-		genphy_suspend(phydev);
-	} else {
-		mask |= ~(BMCR_SPEED1000 | BMCR_FULLDPLX);
-		phy_modify(phydev, MII_BMCR, mask, 0);
-	}
-
 	at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_GREEN,
 			      AT803X_DEBUG_GATE_CLK_IN1000, 0);
 
@@ -1719,6 +1705,27 @@ static int qca83xx_suspend(struct phy_device *phydev)
 	return 0;
 }
 
+static int qca8337_suspend(struct phy_device *phydev)
+{
+	/* Only QCA8337 support actual suspend. */
+	genphy_suspend(phydev);
+
+	return qca83xx_suspend(phydev);
+}
+
+static int qca8327_suspend(struct phy_device *phydev)
+{
+	u16 mask = 0;
+
+	/* QCA8327 cause port unreliability when phy suspend
+	 * is set.
+	 */
+	mask |= ~(BMCR_SPEED1000 | BMCR_FULLDPLX);
+	phy_modify(phydev, MII_BMCR, mask, 0);
+
+	return qca83xx_suspend(phydev);
+}
+
 static int qca808x_phy_fast_retrain_config(struct phy_device *phydev)
 {
 	int ret;
@@ -2180,7 +2187,6 @@ static struct phy_driver at803x_driver[] = {
 	.phy_id_mask		= QCA8K_PHY_ID_MASK,
 	.name			= "Qualcomm Atheros 8337 internal PHY",
 	/* PHY_GBIT_FEATURES */
-	.link_change_notify	= qca83xx_link_change_notify,
 	.probe			= qca83xx_probe,
 	.flags			= PHY_IS_INTERNAL,
 	.config_init		= qca83xx_config_init,
@@ -2188,7 +2194,7 @@ static struct phy_driver at803x_driver[] = {
 	.get_sset_count		= qca83xx_get_sset_count,
 	.get_strings		= qca83xx_get_strings,
 	.get_stats		= qca83xx_get_stats,
-	.suspend		= qca83xx_suspend,
+	.suspend		= qca8337_suspend,
 	.resume			= qca83xx_resume,
 }, {
 	/* QCA8327-A from switch QCA8327-AL1A */
@@ -2199,12 +2205,12 @@ static struct phy_driver at803x_driver[] = {
 	.link_change_notify	= qca83xx_link_change_notify,
 	.probe			= qca83xx_probe,
 	.flags			= PHY_IS_INTERNAL,
-	.config_init		= qca83xx_config_init,
+	.config_init		= qca8327_config_init,
 	.soft_reset		= genphy_soft_reset,
 	.get_sset_count		= qca83xx_get_sset_count,
 	.get_strings		= qca83xx_get_strings,
 	.get_stats		= qca83xx_get_stats,
-	.suspend		= qca83xx_suspend,
+	.suspend		= qca8327_suspend,
 	.resume			= qca83xx_resume,
 }, {
 	/* QCA8327-B from switch QCA8327-BL1A */
@@ -2215,12 +2221,12 @@ static struct phy_driver at803x_driver[] = {
 	.link_change_notify	= qca83xx_link_change_notify,
 	.probe			= qca83xx_probe,
 	.flags			= PHY_IS_INTERNAL,
-	.config_init		= qca83xx_config_init,
+	.config_init		= qca8327_config_init,
 	.soft_reset		= genphy_soft_reset,
 	.get_sset_count		= qca83xx_get_sset_count,
 	.get_strings		= qca83xx_get_strings,
 	.get_stats		= qca83xx_get_stats,
-	.suspend		= qca83xx_suspend,
+	.suspend		= qca8327_suspend,
 	.resume			= qca83xx_resume,
 }, {
 	/* Qualcomm QCA8081 */
-- 
2.40.1

Re: [net-next PATCH 05/14] net: phy: at803x: move qca83xx specific check in dedicated functions

[Andrew Lunn]

On Wed, Nov 29, 2023 at 03:12:10AM +0100, Christian Marangi wrote:
> Rework qca83xx specific check to dedicated function to tidy things up
> and drop useless phy_id check.
> 
> Also drop an useless link_change_notify for QCA8337 as it did nothing an
> returned early.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[Christian Marangi]

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 <ansuelsmth@gmail.com>
---
 drivers/net/phy/at803x.c | 637 ++++++++++++++++++++++-----------------
 1 file changed, 362 insertions(+), 275 deletions(-)

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,
-- 
2.40.1

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

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Christian Marangi]

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? 

-- 
	Ansuel

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

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Andrew Lunn]

> +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

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

[Christian Marangi]

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.
> 
-- 
	Ansuel

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

[Andrew Lunn]

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

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

[Christian Marangi]

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.
>

-- 
	Ansuel

[net-next PATCH 07/14] net: phy: at803x: move at8035 specific DT parse to dedicated probe

[Christian Marangi]

Move at8035 specific DT parse for clock out frequency to dedicated probe
to make at803x probe function more generic.

This is to tidy code and no behaviour change are intended.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/at803x.c | 62 ++++++++++++++++++++++++++++------------
 1 file changed, 43 insertions(+), 19 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index b83422c6db74..e7d006ca1676 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -647,23 +647,6 @@ static int at803x_parse_dt(struct phy_device *phydev)
 
 		priv->clk_25m_reg |= FIELD_PREP(AT803X_CLK_OUT_MASK, sel);
 		priv->clk_25m_mask |= AT803X_CLK_OUT_MASK;
-
-		/* Fixup for the AR8030/AR8035. This chip has another mask and
-		 * doesn't support the DSP reference. Eg. the lowest bit of the
-		 * mask. The upper two bits select the same frequencies. Mask
-		 * the lowest bit here.
-		 *
-		 * Warning:
-		 *   There was no datasheet for the AR8030 available so this is
-		 *   just a guess. But the AR8035 is listed as pin compatible
-		 *   to the AR8030 so there might be a good chance it works on
-		 *   the AR8030 too.
-		 */
-		if (phydev->drv->phy_id == ATH8030_PHY_ID ||
-		    phydev->drv->phy_id == ATH8035_PHY_ID) {
-			priv->clk_25m_reg &= AT8035_CLK_OUT_MASK;
-			priv->clk_25m_mask &= AT8035_CLK_OUT_MASK;
-		}
 	}
 
 	ret = of_property_read_u32(node, "qca,clk-out-strength", &strength);
@@ -1681,6 +1664,47 @@ static int at8031_set_wol(struct phy_device *phydev,
 	return at803x_set_wol(phydev, wol);
 }
 
+static int at8035_parse_dt(struct phy_device *phydev)
+{
+	struct device_node *node = phydev->mdio.dev.of_node;
+	struct at803x_priv *priv = phydev->priv;
+	u32 freq;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_OF_MDIO))
+		return 0;
+
+	ret = of_property_read_u32(node, "qca,clk-out-frequency", &freq);
+	if (!ret) {
+		/* Fixup for the AR8030/AR8035. This chip has another mask and
+		 * doesn't support the DSP reference. Eg. the lowest bit of the
+		 * mask. The upper two bits select the same frequencies. Mask
+		 * the lowest bit here.
+		 *
+		 * Warning:
+		 *   There was no datasheet for the AR8030 available so this is
+		 *   just a guess. But the AR8035 is listed as pin compatible
+		 *   to the AR8030 so there might be a good chance it works on
+		 *   the AR8030 too.
+		 */
+		priv->clk_25m_reg &= AT8035_CLK_OUT_MASK;
+		priv->clk_25m_mask &= AT8035_CLK_OUT_MASK;
+	}
+
+	return 0;
+}
+
+static int at8035_probe(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = at803x_probe(phydev);
+	if (ret)
+		return ret;
+
+	return at8035_parse_dt(phydev);
+}
+
 static int qca83xx_probe(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;
@@ -2167,7 +2191,7 @@ static struct phy_driver at803x_driver[] = {
 	PHY_ID_MATCH_EXACT(ATH8035_PHY_ID),
 	.name			= "Qualcomm Atheros AR8035",
 	.flags			= PHY_POLL_CABLE_TEST,
-	.probe			= at803x_probe,
+	.probe			= at8035_probe,
 	.config_aneg		= at803x_config_aneg,
 	.config_init		= at803x_config_init,
 	.soft_reset		= genphy_soft_reset,
@@ -2188,7 +2212,7 @@ static struct phy_driver at803x_driver[] = {
 	.phy_id			= ATH8030_PHY_ID,
 	.name			= "Qualcomm Atheros AR8030",
 	.phy_id_mask		= AT8030_PHY_ID_MASK,
-	.probe			= at803x_probe,
+	.probe			= at8035_probe,
 	.config_init		= at803x_config_init,
 	.link_change_notify	= at803x_link_change_notify,
 	.set_wol		= at803x_set_wol,
-- 
2.40.1

Re: [net-next PATCH 07/14] net: phy: at803x: move at8035 specific DT parse to dedicated probe

[Andrew Lunn]

> +static int at8035_parse_dt(struct phy_device *phydev)
> +{
> +	struct device_node *node = phydev->mdio.dev.of_node;
> +	struct at803x_priv *priv = phydev->priv;
> +	u32 freq;
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_OF_MDIO))
> +		return 0;
> +
> +	ret = of_property_read_u32(node, "qca,clk-out-frequency", &freq);
> +	if (!ret) {

I don't think you need this. priv->clk_25m_reg and priv->clk_25m_mask
will default to 0. If qca,clk-out-frequency does not exist, they will
still be zero....

> +		/* Fixup for the AR8030/AR8035. This chip has another mask and
> +		 * doesn't support the DSP reference. Eg. the lowest bit of the
> +		 * mask. The upper two bits select the same frequencies. Mask
> +		 * the lowest bit here.
> +		 *
> +		 * Warning:
> +		 *   There was no datasheet for the AR8030 available so this is
> +		 *   just a guess. But the AR8035 is listed as pin compatible
> +		 *   to the AR8030 so there might be a good chance it works on
> +		 *   the AR8030 too.
> +		 */
> +		priv->clk_25m_reg &= AT8035_CLK_OUT_MASK;
> +		priv->clk_25m_mask &= AT8035_CLK_OUT_MASK;

... so applying a mask to 0 does nothing.

It does change the code a little, but you can add a justification in
the commit message.

    Andrew

[net-next PATCH 08/14] net: phy: at803x: drop specific PHY id check from cable test functions

[Christian Marangi]

Drop specific PHY id check for cable test functions for at803x. This is
done to make functions more generic.

PHYs that requires to set additional reg are moved to specific function
calling the more generic one.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/at803x.c | 48 +++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index e7d006ca1676..8f5878ccb1a8 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -1263,19 +1263,11 @@ static int at803x_cable_test_one_pair(struct phy_device *phydev, int pair)
 }
 
 static int at803x_cable_test_get_status(struct phy_device *phydev,
-					bool *finished)
+					bool *finished, unsigned long pair_mask)
 {
-	unsigned long pair_mask;
 	int retries = 20;
 	int pair, ret;
 
-	if (phydev->phy_id == ATH9331_PHY_ID ||
-	    phydev->phy_id == ATH8032_PHY_ID ||
-	    phydev->phy_id == QCA9561_PHY_ID)
-		pair_mask = 0x3;
-	else
-		pair_mask = 0xf;
-
 	*finished = false;
 
 	/* According to the datasheet the CDT can be performed when
@@ -1310,10 +1302,6 @@ static int at803x_cable_test_start(struct phy_device *phydev)
 	 */
 	phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
 	phy_write(phydev, MII_ADVERTISE, ADVERTISE_CSMA);
-	if (phydev->phy_id != ATH9331_PHY_ID &&
-	    phydev->phy_id != ATH8032_PHY_ID &&
-	    phydev->phy_id != QCA9561_PHY_ID)
-		phy_write(phydev, MII_CTRL1000, 0);
 
 	/* we do all the (time consuming) work later */
 	return 0;
@@ -1664,6 +1652,26 @@ static int at8031_set_wol(struct phy_device *phydev,
 	return at803x_set_wol(phydev, wol);
 }
 
+static int at8031_cable_test_get_status(struct phy_device *phydev,
+					bool *finished)
+{
+	return at803x_cable_test_get_status(phydev, finished, 0xf);
+}
+
+static int at8031_cable_test_start(struct phy_device *phydev)
+{
+	at803x_cable_test_start(phydev);
+	phy_write(phydev, MII_CTRL1000, 0);
+
+	return 0;
+}
+
+static int at8032_cable_test_get_status(struct phy_device *phydev,
+					bool *finished)
+{
+	return at803x_cable_test_get_status(phydev, finished, 0x3);
+}
+
 static int at8035_parse_dt(struct phy_device *phydev)
 {
 	struct device_node *node = phydev->mdio.dev.of_node;
@@ -2205,8 +2213,8 @@ static struct phy_driver at803x_driver[] = {
 	.handle_interrupt	= at803x_handle_interrupt,
 	.get_tunable		= at803x_get_tunable,
 	.set_tunable		= at803x_set_tunable,
-	.cable_test_start	= at803x_cable_test_start,
-	.cable_test_get_status	= at803x_cable_test_get_status,
+	.cable_test_start	= at8031_cable_test_start,
+	.cable_test_get_status	= at8031_cable_test_get_status,
 }, {
 	/* Qualcomm Atheros AR8030 */
 	.phy_id			= ATH8030_PHY_ID,
@@ -2243,8 +2251,8 @@ static struct phy_driver at803x_driver[] = {
 	.handle_interrupt	= at803x_handle_interrupt,
 	.get_tunable		= at803x_get_tunable,
 	.set_tunable		= at803x_set_tunable,
-	.cable_test_start	= at803x_cable_test_start,
-	.cable_test_get_status	= at803x_cable_test_get_status,
+	.cable_test_start	= at8031_cable_test_start,
+	.cable_test_get_status	= at8031_cable_test_get_status,
 }, {
 	/* Qualcomm Atheros AR8032 */
 	PHY_ID_MATCH_EXACT(ATH8032_PHY_ID),
@@ -2259,7 +2267,7 @@ static struct phy_driver at803x_driver[] = {
 	.config_intr		= at803x_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
 	.cable_test_start	= at803x_cable_test_start,
-	.cable_test_get_status	= at803x_cable_test_get_status,
+	.cable_test_get_status	= at8032_cable_test_get_status,
 }, {
 	/* ATHEROS AR9331 */
 	PHY_ID_MATCH_EXACT(ATH9331_PHY_ID),
@@ -2272,7 +2280,7 @@ static struct phy_driver at803x_driver[] = {
 	.config_intr		= at803x_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
 	.cable_test_start	= at803x_cable_test_start,
-	.cable_test_get_status	= at803x_cable_test_get_status,
+	.cable_test_get_status	= at8032_cable_test_get_status,
 	.read_status		= at803x_read_status,
 	.soft_reset		= genphy_soft_reset,
 	.config_aneg		= at803x_config_aneg,
@@ -2288,7 +2296,7 @@ static struct phy_driver at803x_driver[] = {
 	.config_intr		= at803x_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
 	.cable_test_start	= at803x_cable_test_start,
-	.cable_test_get_status	= at803x_cable_test_get_status,
+	.cable_test_get_status	= at8032_cable_test_get_status,
 	.read_status		= at803x_read_status,
 	.soft_reset		= genphy_soft_reset,
 	.config_aneg		= at803x_config_aneg,
-- 
2.40.1

Re: [net-next PATCH 08/14] net: phy: at803x: drop specific PHY id check from cable test functions

[Russell King (Oracle)]

On Wed, Nov 29, 2023 at 03:12:13AM +0100, Christian Marangi wrote:
> @@ -1310,10 +1302,6 @@ static int at803x_cable_test_start(struct phy_device *phydev)
>  	 */
>  	phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
>  	phy_write(phydev, MII_ADVERTISE, ADVERTISE_CSMA);
> -	if (phydev->phy_id != ATH9331_PHY_ID &&
> -	    phydev->phy_id != ATH8032_PHY_ID &&
> -	    phydev->phy_id != QCA9561_PHY_ID)
> -		phy_write(phydev, MII_CTRL1000, 0);
...
> +static int at8031_cable_test_start(struct phy_device *phydev)
> +{
> +	at803x_cable_test_start(phydev);
> +	phy_write(phydev, MII_CTRL1000, 0);

I don't think this is a safe change - same reasons as given on a
previous patch. You can't randomly reorder register writes like this.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [net-next PATCH 08/14] net: phy: at803x: drop specific PHY id check from cable test functions

[Christian Marangi]

On Wed, Nov 29, 2023 at 09:38:39AM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 29, 2023 at 03:12:13AM +0100, Christian Marangi wrote:
> > @@ -1310,10 +1302,6 @@ static int at803x_cable_test_start(struct phy_device *phydev)
> >  	 */
> >  	phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
> >  	phy_write(phydev, MII_ADVERTISE, ADVERTISE_CSMA);
> > -	if (phydev->phy_id != ATH9331_PHY_ID &&
> > -	    phydev->phy_id != ATH8032_PHY_ID &&
> > -	    phydev->phy_id != QCA9561_PHY_ID)
> > -		phy_write(phydev, MII_CTRL1000, 0);
> ...
> > +static int at8031_cable_test_start(struct phy_device *phydev)
> > +{
> > +	at803x_cable_test_start(phydev);
> > +	phy_write(phydev, MII_CTRL1000, 0);
> 
> I don't think this is a safe change - same reasons as given on a
> previous patch. You can't randomly reorder register writes like this.
>

Actually for this the order is keeped. Generic function is called and
for at8031 MII_CTRL1000 is called on top of that.

-- 
	Ansuel

Re: [net-next PATCH 08/14] net: phy: at803x: drop specific PHY id check from cable test functions

[Russell King (Oracle)]

On Wed, Nov 29, 2023 at 10:47:18AM +0100, Christian Marangi wrote:
> On Wed, Nov 29, 2023 at 09:38:39AM +0000, Russell King (Oracle) wrote:
> > On Wed, Nov 29, 2023 at 03:12:13AM +0100, Christian Marangi wrote:
> > > @@ -1310,10 +1302,6 @@ static int at803x_cable_test_start(struct phy_device *phydev)
> > >  	 */
> > >  	phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
> > >  	phy_write(phydev, MII_ADVERTISE, ADVERTISE_CSMA);
> > > -	if (phydev->phy_id != ATH9331_PHY_ID &&
> > > -	    phydev->phy_id != ATH8032_PHY_ID &&
> > > -	    phydev->phy_id != QCA9561_PHY_ID)
> > > -		phy_write(phydev, MII_CTRL1000, 0);
> > ...
> > > +static int at8031_cable_test_start(struct phy_device *phydev)
> > > +{
> > > +	at803x_cable_test_start(phydev);
> > > +	phy_write(phydev, MII_CTRL1000, 0);
> > 
> > I don't think this is a safe change - same reasons as given on a
> > previous patch. You can't randomly reorder register writes like this.
> >
> 
> Actually for this the order is keeped. Generic function is called and
> for at8031 MII_CTRL1000 is called on top of that.

Okay, but I don't like it. I would prefer this to be:

static void at803x_cable_test_autoneg(struct phy_device *phydev)
{
	phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
	phy_write(phydev, MII_ADVERTISE, ADVERTISE_CSMA);
}

static int at803x_cable_test_start(struct phy_device *phydev)
{
	at803x_cable_test_autoneg(phydev);
	return 0;
}

static int at8031_cable_test_start(struct phy_device *phydev)
{
	at803x_cable_test_autoneg(phydev);
	phy_write(phydev, MII_CTRL1000, 0);
	return 0;
}

which makes it more explicit what is going on here. Also a comment
above the function stating that it's for AR8031 _and_ AR8035 would
be useful.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [net-next PATCH 08/14] net: phy: at803x: drop specific PHY id check from cable test functions

[Christian Marangi]

On Wed, Nov 29, 2023 at 10:57:28AM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 29, 2023 at 10:47:18AM +0100, Christian Marangi wrote:
> > On Wed, Nov 29, 2023 at 09:38:39AM +0000, Russell King (Oracle) wrote:
> > > On Wed, Nov 29, 2023 at 03:12:13AM +0100, Christian Marangi wrote:
> > > > @@ -1310,10 +1302,6 @@ static int at803x_cable_test_start(struct phy_device *phydev)
> > > >  	 */
> > > >  	phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
> > > >  	phy_write(phydev, MII_ADVERTISE, ADVERTISE_CSMA);
> > > > -	if (phydev->phy_id != ATH9331_PHY_ID &&
> > > > -	    phydev->phy_id != ATH8032_PHY_ID &&
> > > > -	    phydev->phy_id != QCA9561_PHY_ID)
> > > > -		phy_write(phydev, MII_CTRL1000, 0);
> > > ...
> > > > +static int at8031_cable_test_start(struct phy_device *phydev)
> > > > +{
> > > > +	at803x_cable_test_start(phydev);
> > > > +	phy_write(phydev, MII_CTRL1000, 0);
> > > 
> > > I don't think this is a safe change - same reasons as given on a
> > > previous patch. You can't randomly reorder register writes like this.
> > >
> > 
> > Actually for this the order is keeped. Generic function is called and
> > for at8031 MII_CTRL1000 is called on top of that.
> 
> Okay, but I don't like it. I would prefer this to be:
> 
> static void at803x_cable_test_autoneg(struct phy_device *phydev)
> {
> 	phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
> 	phy_write(phydev, MII_ADVERTISE, ADVERTISE_CSMA);
> }
> 
> static int at803x_cable_test_start(struct phy_device *phydev)
> {
> 	at803x_cable_test_autoneg(phydev);
> 	return 0;
> }
> 
> static int at8031_cable_test_start(struct phy_device *phydev)
> {
> 	at803x_cable_test_autoneg(phydev);
> 	phy_write(phydev, MII_CTRL1000, 0);
> 	return 0;
> }
> 
> which makes it more explicit what is going on here. Also a comment
> above the function stating that it's for AR8031 _and_ AR8035 would
> be useful.
>

Much cleaner thanks for the hint!

-- 
	Ansuel

Re: [net-next PATCH 08/14] net: phy: at803x: drop specific PHY id check from cable test functions

[Russell King (Oracle)]

Andrew,

On Wed, Nov 29, 2023 at 03:12:13AM +0100, Christian Marangi wrote:
>  static int at8035_parse_dt(struct phy_device *phydev)
>  {
>  	struct device_node *node = phydev->mdio.dev.of_node;
> @@ -2205,8 +2213,8 @@ static struct phy_driver at803x_driver[] = {
>  	.handle_interrupt	= at803x_handle_interrupt,
>  	.get_tunable		= at803x_get_tunable,
>  	.set_tunable		= at803x_set_tunable,
> -	.cable_test_start	= at803x_cable_test_start,
> -	.cable_test_get_status	= at803x_cable_test_get_status,
> +	.cable_test_start	= at8031_cable_test_start,
> +	.cable_test_get_status	= at8031_cable_test_get_status,
>  }, {
>  	/* Qualcomm Atheros AR8030 */
>  	.phy_id			= ATH8030_PHY_ID,
> @@ -2243,8 +2251,8 @@ static struct phy_driver at803x_driver[] = {
>  	.handle_interrupt	= at803x_handle_interrupt,
>  	.get_tunable		= at803x_get_tunable,
>  	.set_tunable		= at803x_set_tunable,
> -	.cable_test_start	= at803x_cable_test_start,
> -	.cable_test_get_status	= at803x_cable_test_get_status,
> +	.cable_test_start	= at8031_cable_test_start,
> +	.cable_test_get_status	= at8031_cable_test_get_status,
>  }, {
>  	/* Qualcomm Atheros AR8032 */
>  	PHY_ID_MATCH_EXACT(ATH8032_PHY_ID),
> @@ -2259,7 +2267,7 @@ static struct phy_driver at803x_driver[] = {
>  	.config_intr		= at803x_config_intr,
>  	.handle_interrupt	= at803x_handle_interrupt,
>  	.cable_test_start	= at803x_cable_test_start,
> -	.cable_test_get_status	= at803x_cable_test_get_status,
> +	.cable_test_get_status	= at8032_cable_test_get_status,
>  }, {
>  	/* ATHEROS AR9331 */
>  	PHY_ID_MATCH_EXACT(ATH9331_PHY_ID),
> @@ -2272,7 +2280,7 @@ static struct phy_driver at803x_driver[] = {
>  	.config_intr		= at803x_config_intr,
>  	.handle_interrupt	= at803x_handle_interrupt,
>  	.cable_test_start	= at803x_cable_test_start,
> -	.cable_test_get_status	= at803x_cable_test_get_status,
> +	.cable_test_get_status	= at8032_cable_test_get_status,
>  	.read_status		= at803x_read_status,
>  	.soft_reset		= genphy_soft_reset,
>  	.config_aneg		= at803x_config_aneg,
> @@ -2288,7 +2296,7 @@ static struct phy_driver at803x_driver[] = {
>  	.config_intr		= at803x_config_intr,
>  	.handle_interrupt	= at803x_handle_interrupt,
>  	.cable_test_start	= at803x_cable_test_start,
> -	.cable_test_get_status	= at803x_cable_test_get_status,
> +	.cable_test_get_status	= at8032_cable_test_get_status,
>  	.read_status		= at803x_read_status,
>  	.soft_reset		= genphy_soft_reset,
>  	.config_aneg		= at803x_config_aneg,

We could _really_ do with moving away from an array of PHY driver
structures in phylib because patches like this are hard to properly
review. The problem is there is little context to say _which_ driver
instance is being changed. The only thing that saves us above are
the comments on the next instance - but those may not be present
if we're modifying something in the middle of each definition.

The same issue happens with the mv88e6xxx driver, with that big
array in chip.c, where we have loads of function pointers. It's
far from ideal.

Maybe we should consider moving to a model where each driver is
defined as a separate named structure, and then we have an array
of pointers to each driver, which is then passed into a new PHY
driver registration function? This way, at least the @@ line will
identify to a reviewer which instance is being modified.

This won't help the problem of a patch being mis-applied due to
there not being sufficient differences in context, but if one
subsequently diffs after applying such a change and compares the
patch to the original, there will be a difference in the @@ line.
(However, arguably that level of checking is unlikely to happen.)


-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[net-next PATCH 09/14] net: phy: at803x: remove specific qca808x check from at803x functions

[Christian Marangi]

Remove specific qca808x check from at803x generic functions.

While this cause a bit of code duplication, this is needed in
preparation for splitting the driver per PHY family and detaching
qca808x specific bits from the at803x driver.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/at803x.c | 107 ++++++++++++++++++++++++++-------------
 1 file changed, 71 insertions(+), 36 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 8f5878ccb1a8..475b96165f45 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -1043,24 +1043,6 @@ static int at803x_config_aneg(struct phy_device *phydev)
 	 */
 	ret = 0;
 
-	if (phydev->drv->phy_id == QCA8081_PHY_ID) {
-		int phy_ctrl = 0;
-
-		/* The reg MII_BMCR also needs to be configured for force mode, the
-		 * genphy_config_aneg is also needed.
-		 */
-		if (phydev->autoneg == AUTONEG_DISABLE)
-			genphy_c45_pma_setup_forced(phydev);
-
-		if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->advertising))
-			phy_ctrl = MDIO_AN_10GBT_CTRL_ADV2_5G;
-
-		ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
-				MDIO_AN_10GBT_CTRL_ADV2_5G, phy_ctrl);
-		if (ret < 0)
-			return ret;
-	}
-
 	return __genphy_config_aneg(phydev, ret);
 }
 
@@ -1197,14 +1179,8 @@ static int at803x_cdt_start(struct phy_device *phydev, int pair)
 {
 	u16 cdt;
 
-	/* qca8081 takes the different bit 15 to enable CDT test */
-	if (phydev->drv->phy_id == QCA8081_PHY_ID)
-		cdt = QCA808X_CDT_ENABLE_TEST |
-			QCA808X_CDT_LENGTH_UNIT |
-			QCA808X_CDT_INTER_CHECK_DIS;
-	else
-		cdt = FIELD_PREP(AT803X_CDT_MDI_PAIR_MASK, pair) |
-			AT803X_CDT_ENABLE_TEST;
+	cdt = FIELD_PREP(AT803X_CDT_MDI_PAIR_MASK, pair) |
+	      AT803X_CDT_ENABLE_TEST;
 
 	return phy_write(phydev, AT803X_CDT, cdt);
 }
@@ -1212,16 +1188,10 @@ static int at803x_cdt_start(struct phy_device *phydev, int pair)
 static int at803x_cdt_wait_for_completion(struct phy_device *phydev)
 {
 	int val, ret;
-	u16 cdt_en;
-
-	if (phydev->drv->phy_id == QCA8081_PHY_ID)
-		cdt_en = QCA808X_CDT_ENABLE_TEST;
-	else
-		cdt_en = AT803X_CDT_ENABLE_TEST;
 
 	/* One test run takes about 25ms */
 	ret = phy_read_poll_timeout(phydev, AT803X_CDT, val,
-				    !(val & cdt_en),
+				    !(val & AT803X_CDT_ENABLE_TEST),
 				    30000, 100000, true);
 
 	return ret < 0 ? ret : 0;
@@ -1845,6 +1815,47 @@ static int qca8327_suspend(struct phy_device *phydev)
 	return qca83xx_suspend(phydev);
 }
 
+static int qca808x_config_aneg(struct phy_device *phydev)
+{
+	int phy_ctrl = 0;
+	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;
+	}
+
+	/* Do not restart auto-negotiation by setting ret to 0 defautly,
+	 * when calling __genphy_config_aneg later.
+	 */
+	ret = 0;
+
+	/* The reg MII_BMCR also needs to be configured for force mode, the
+	 * genphy_config_aneg is also needed.
+	 */
+	if (phydev->autoneg == AUTONEG_DISABLE)
+		genphy_c45_pma_setup_forced(phydev);
+
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->advertising))
+		phy_ctrl = MDIO_AN_10GBT_CTRL_ADV2_5G;
+
+	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
+				     MDIO_AN_10GBT_CTRL_ADV2_5G, phy_ctrl);
+	if (ret < 0)
+		return ret;
+
+	return __genphy_config_aneg(phydev, ret);
+}
+
 static int qca808x_phy_fast_retrain_config(struct phy_device *phydev)
 {
 	int ret;
@@ -2104,6 +2115,30 @@ static int qca808x_cable_test_start(struct phy_device *phydev)
 	return 0;
 }
 
+static int qca808x_cdt_start(struct phy_device *phydev)
+{
+	u16 cdt;
+
+	/* qca8081 takes the different bit 15 to enable CDT test */
+	cdt = QCA808X_CDT_ENABLE_TEST |
+	      QCA808X_CDT_LENGTH_UNIT |
+	      QCA808X_CDT_INTER_CHECK_DIS;
+
+	return phy_write(phydev, AT803X_CDT, cdt);
+}
+
+static int qca808x_cdt_wait_for_completition(struct phy_device *phydev)
+{
+	int val, ret;
+
+	/* One test run takes about 25ms */
+	ret = phy_read_poll_timeout(phydev, AT803X_CDT, val,
+				    !(val & QCA808X_CDT_ENABLE_TEST),
+				    30000, 100000, true);
+
+	return ret < 0 ? ret : 0;
+}
+
 static int qca808x_cable_test_get_status(struct phy_device *phydev, bool *finished)
 {
 	int ret, val;
@@ -2111,11 +2146,11 @@ static int qca808x_cable_test_get_status(struct phy_device *phydev, bool *finish
 
 	*finished = false;
 
-	ret = at803x_cdt_start(phydev, 0);
+	ret = qca808x_cdt_start(phydev);
 	if (ret)
 		return ret;
 
-	ret = at803x_cdt_wait_for_completion(phydev);
+	ret = qca808x_cdt_wait_for_completition(phydev);
 	if (ret)
 		return ret;
 
@@ -2360,7 +2395,7 @@ static struct phy_driver at803x_driver[] = {
 	.set_wol		= at803x_set_wol,
 	.get_wol		= at803x_get_wol,
 	.get_features		= qca808x_get_features,
-	.config_aneg		= at803x_config_aneg,
+	.config_aneg		= qca808x_config_aneg,
 	.suspend		= genphy_suspend,
 	.resume			= genphy_resume,
 	.read_status		= qca808x_read_status,
-- 
2.40.1

Re: [net-next PATCH 09/14] net: phy: at803x: remove specific qca808x check from at803x functions

[Russell King (Oracle)]

On Wed, Nov 29, 2023 at 03:12:14AM +0100, Christian Marangi wrote:
> Remove specific qca808x check from at803x generic functions.
> 
> While this cause a bit of code duplication, this is needed in
> preparation for splitting the driver per PHY family and detaching
> qca808x specific bits from the at803x driver.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/phy/at803x.c | 107 ++++++++++++++++++++++++++-------------
>  1 file changed, 71 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 8f5878ccb1a8..475b96165f45 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -1043,24 +1043,6 @@ static int at803x_config_aneg(struct phy_device *phydev)
>  	 */
>  	ret = 0;

Doesn't this become unnecessary?
>  
> -	if (phydev->drv->phy_id == QCA8081_PHY_ID) {
> -		int phy_ctrl = 0;
> -
> -		/* The reg MII_BMCR also needs to be configured for force mode, the
> -		 * genphy_config_aneg is also needed.
> -		 */
> -		if (phydev->autoneg == AUTONEG_DISABLE)
> -			genphy_c45_pma_setup_forced(phydev);
> -
> -		if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->advertising))
> -			phy_ctrl = MDIO_AN_10GBT_CTRL_ADV2_5G;
> -
> -		ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
> -				MDIO_AN_10GBT_CTRL_ADV2_5G, phy_ctrl);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
>  	return __genphy_config_aneg(phydev, ret);

... since you can just call genphy_config_aneg() here now?

> @@ -1845,6 +1815,47 @@ static int qca8327_suspend(struct phy_device *phydev)
>  	return qca83xx_suspend(phydev);
>  }
>  
> +static int qca808x_config_aneg(struct phy_device *phydev)
> +{
> +	int phy_ctrl = 0;
> +	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;
> +	}
> +
> +	/* Do not restart auto-negotiation by setting ret to 0 defautly,
> +	 * when calling __genphy_config_aneg later.
> +	 */
> +	ret = 0;
> +
> +	/* The reg MII_BMCR also needs to be configured for force mode, the
> +	 * genphy_config_aneg is also needed.
> +	 */
> +	if (phydev->autoneg == AUTONEG_DISABLE)
> +		genphy_c45_pma_setup_forced(phydev);
> +
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->advertising))
> +		phy_ctrl = MDIO_AN_10GBT_CTRL_ADV2_5G;
> +
> +	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
> +				     MDIO_AN_10GBT_CTRL_ADV2_5G, phy_ctrl);
> +	if (ret < 0)
> +		return ret;
> +
> +	return __genphy_config_aneg(phydev, ret);
> +}

... but is it _really_ worth duplicating the entire function just to
deal with the QCA8081 difference? On balance, I think the original code
is better.

Overall, I'm getting the impression that you have a mental hang-up about
drivers checking the PHY ID in their method drivers... there's
absolutely nothing wrong with that. When the result of trying to
eliminate those results in bloating a driver, then the cleanup is not
a cleanup anymore, it creates bloat and makes future maintenance
harder.

Sorry, but no, I don't like this patch.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [net-next PATCH 09/14] net: phy: at803x: remove specific qca808x check from at803x functions

[Christian Marangi]

On Wed, Nov 29, 2023 at 09:43:40AM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 29, 2023 at 03:12:14AM +0100, Christian Marangi wrote:
> > Remove specific qca808x check from at803x generic functions.
> > 
> > While this cause a bit of code duplication, this is needed in
> > preparation for splitting the driver per PHY family and detaching
> > qca808x specific bits from the at803x driver.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/net/phy/at803x.c | 107 ++++++++++++++++++++++++++-------------
> >  1 file changed, 71 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> > index 8f5878ccb1a8..475b96165f45 100644
> > --- a/drivers/net/phy/at803x.c
> > +++ b/drivers/net/phy/at803x.c
> > @@ -1043,24 +1043,6 @@ static int at803x_config_aneg(struct phy_device *phydev)
> >  	 */
> >  	ret = 0;
> 
> Doesn't this become unnecessary?
> >  
> > -	if (phydev->drv->phy_id == QCA8081_PHY_ID) {
> > -		int phy_ctrl = 0;
> > -
> > -		/* The reg MII_BMCR also needs to be configured for force mode, the
> > -		 * genphy_config_aneg is also needed.
> > -		 */
> > -		if (phydev->autoneg == AUTONEG_DISABLE)
> > -			genphy_c45_pma_setup_forced(phydev);
> > -
> > -		if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->advertising))
> > -			phy_ctrl = MDIO_AN_10GBT_CTRL_ADV2_5G;
> > -
> > -		ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
> > -				MDIO_AN_10GBT_CTRL_ADV2_5G, phy_ctrl);
> > -		if (ret < 0)
> > -			return ret;
> > -	}
> > -
> >  	return __genphy_config_aneg(phydev, ret);
> 
> ... since you can just call genphy_config_aneg() here now?
> 
> > @@ -1845,6 +1815,47 @@ static int qca8327_suspend(struct phy_device *phydev)
> >  	return qca83xx_suspend(phydev);
> >  }
> >  
> > +static int qca808x_config_aneg(struct phy_device *phydev)
> > +{
> > +	int phy_ctrl = 0;
> > +	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;
> > +	}
> > +
> > +	/* Do not restart auto-negotiation by setting ret to 0 defautly,
> > +	 * when calling __genphy_config_aneg later.
> > +	 */
> > +	ret = 0;
> > +
> > +	/* The reg MII_BMCR also needs to be configured for force mode, the
> > +	 * genphy_config_aneg is also needed.
> > +	 */
> > +	if (phydev->autoneg == AUTONEG_DISABLE)
> > +		genphy_c45_pma_setup_forced(phydev);
> > +
> > +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->advertising))
> > +		phy_ctrl = MDIO_AN_10GBT_CTRL_ADV2_5G;
> > +
> > +	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
> > +				     MDIO_AN_10GBT_CTRL_ADV2_5G, phy_ctrl);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return __genphy_config_aneg(phydev, ret);
> > +}
> 
> ... but is it _really_ worth duplicating the entire function just to
> deal with the QCA8081 difference? On balance, I think the original code
> is better.
> 
> Overall, I'm getting the impression that you have a mental hang-up about
> drivers checking the PHY ID in their method drivers... there's
> absolutely nothing wrong with that. When the result of trying to
> eliminate those results in bloating a driver, then the cleanup is not
> a cleanup anymore, it creates bloat and makes future maintenance
> harder.

For some AT803x ID it might be O.K. but here we are mixing all kind of
thing and you already noticing the state of this driver with the priv
changes. Again it's all to facilitate the last 2 patch of this series.

> 
> Sorry, but no, I don't like this patch.

-- 
	Ansuel

[net-next PATCH 10/14] net: phy: at803x: drop usless probe for qca8081 PHY

[Christian Marangi]

Drop useless probe for qca8081 PHY. The specific functions and the
generic ones doesn't use any of allocated variables of the at803x_priv
struct and doesn't support any of the properties used for at803x PHYs.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/at803x.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 475b96165f45..32f44ef9835b 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -2387,7 +2387,6 @@ static struct phy_driver at803x_driver[] = {
 	PHY_ID_MATCH_EXACT(QCA8081_PHY_ID),
 	.name			= "Qualcomm QCA8081",
 	.flags			= PHY_POLL_CABLE_TEST,
-	.probe			= at803x_probe,
 	.config_intr		= at803x_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
 	.get_tunable		= at803x_get_tunable,
-- 
2.40.1

Re: [net-next PATCH 10/14] net: phy: at803x: drop usless probe for qca8081 PHY

[Russell King (Oracle)]

On Wed, Nov 29, 2023 at 03:12:15AM +0100, Christian Marangi wrote:
> Drop useless probe for qca8081 PHY. The specific functions and the
> generic ones doesn't use any of allocated variables of the at803x_priv
> struct and doesn't support any of the properties used for at803x PHYs.

So now we have two different structures in ->priv _and_ ->priv can be
NULL all in the same driver.

This is getting rediculous.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [net-next PATCH 10/14] net: phy: at803x: drop usless probe for qca8081 PHY

[Christian Marangi]

On Wed, Nov 29, 2023 at 09:44:42AM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 29, 2023 at 03:12:15AM +0100, Christian Marangi wrote:
> > Drop useless probe for qca8081 PHY. The specific functions and the
> > generic ones doesn't use any of allocated variables of the at803x_priv
> > struct and doesn't support any of the properties used for at803x PHYs.
> 
> So now we have two different structures in ->priv _and_ ->priv can be
> NULL all in the same driver.
> 
> This is getting rediculous.
>

Saddly this is the state of this PHY driver... Imagine me noticing that
qca808x actually don't use any of the priv struct and doen't support any
of the proprerty parsed in the OF function...

Guess I have to move also this change where I split the driver.
(again trying to keep change as little as possible)

-- 
	Ansuel

[net-next PATCH 11/14] net: phy: at803x: make specific status mask more generic

[Christian Marangi]

Rework specific status masks to be more generic and drop checking
PHY ID in read_specific_status.

The function now takes an additional arg where the speed mask is passed
and then extracted from the AT803X_SPECIFIC_STATUS reg.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/at803x.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 32f44ef9835b..c6aa31495324 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -301,6 +301,11 @@ static struct at803x_hw_stat qca83xx_hw_stats[] = {
 	{ "eee_wake_errors", 0x16, GENMASK(15, 0), MMD},
 };
 
+struct at803x_ss_mask {
+	u16 speed_mask;
+	u8 speed_shift;
+};
+
 struct qca83xx_priv {
 	u64 stats[ARRAY_SIZE(qca83xx_hw_stats)];
 };
@@ -900,7 +905,8 @@ static void at803x_link_change_notify(struct phy_device *phydev)
 	}
 }
 
-static int at803x_read_specific_status(struct phy_device *phydev)
+static int at803x_read_specific_status(struct phy_device *phydev,
+				       struct at803x_ss_mask ss_mask)
 {
 	int ss;
 
@@ -919,11 +925,8 @@ static int at803x_read_specific_status(struct phy_device *phydev)
 		if (sfc < 0)
 			return sfc;
 
-		/* qca8081 takes the different bits for speed value from at803x */
-		if (phydev->drv->phy_id == QCA8081_PHY_ID)
-			speed = FIELD_GET(QCA808X_SS_SPEED_MASK, ss);
-		else
-			speed = FIELD_GET(AT803X_SS_SPEED_MASK, ss);
+		speed = ss & ss_mask.speed_mask;
+		speed >>= ss_mask.speed_shift;
 
 		switch (speed) {
 		case AT803X_SS_SPEED_10:
@@ -967,6 +970,7 @@ static int at803x_read_specific_status(struct phy_device *phydev)
 
 static int at803x_read_status(struct phy_device *phydev)
 {
+	struct at803x_ss_mask ss_mask = { 0 };
 	int err, old_link = phydev->link;
 
 	/* Update the link, but return if there was an error */
@@ -987,7 +991,9 @@ static int at803x_read_status(struct phy_device *phydev)
 	if (err < 0)
 		return err;
 
-	err = at803x_read_specific_status(phydev);
+	ss_mask.speed_mask = AT803X_SS_SPEED_MASK;
+	ss_mask.speed_shift = __bf_shf(AT803X_SS_SPEED_MASK);
+	err = at803x_read_specific_status(phydev, ss_mask);
 	if (err < 0)
 		return err;
 
@@ -1960,6 +1966,7 @@ static int qca808x_config_init(struct phy_device *phydev)
 
 static int qca808x_read_status(struct phy_device *phydev)
 {
+	struct at803x_ss_mask ss_mask = { 0 };
 	int ret;
 
 	ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_STAT);
@@ -1973,7 +1980,10 @@ static int qca808x_read_status(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	ret = at803x_read_specific_status(phydev);
+	/* qca8081 takes the different bits for speed value from at803x */
+	ss_mask.speed_mask = QCA808X_SS_SPEED_MASK;
+	ss_mask.speed_shift = __bf_shf(QCA808X_SS_SPEED_MASK);
+	ret = at803x_read_specific_status(phydev, ss_mask);
 	if (ret < 0)
 		return ret;
 
-- 
2.40.1

[net-next PATCH 12/14] net: phy: move at803x PHY driver to dedicated directory

[Christian Marangi]

In preparation for addition of other Qcom PHY and to tidy things up,
move the at803x PHY driver to dedicated directory.

The same order in the Kconfig selection is saved.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/Kconfig             | 7 +------
 drivers/net/phy/Makefile            | 2 +-
 drivers/net/phy/qcom/Kconfig        | 7 +++++++
 drivers/net/phy/qcom/Makefile       | 2 ++
 drivers/net/phy/{ => qcom}/at803x.c | 0
 5 files changed, 11 insertions(+), 7 deletions(-)
 create mode 100644 drivers/net/phy/qcom/Kconfig
 create mode 100644 drivers/net/phy/qcom/Makefile
 rename drivers/net/phy/{ => qcom}/at803x.c (100%)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 25cfc5ded1da..c47fe2302150 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -318,12 +318,7 @@ config NCN26000_PHY
 	  Currently supports the NCN26000 10BASE-T1S Industrial PHY
 	  with MII interface.
 
-config AT803X_PHY
-	tristate "Qualcomm Atheros AR803X PHYs and QCA833x PHYs"
-	depends on REGULATOR
-	help
-	  Currently supports the AR8030, AR8031, AR8033, AR8035 and internal
-	  QCA8337(Internal qca8k PHY) model
+source "drivers/net/phy/qcom/Kconfig"
 
 config QSEMI_PHY
 	tristate "Quality Semiconductor PHYs"
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index f65e85c91fc1..4ecf704d0e4b 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -36,7 +36,7 @@ obj-$(CONFIG_ADIN_PHY)		+= adin.o
 obj-$(CONFIG_ADIN1100_PHY)	+= adin1100.o
 obj-$(CONFIG_AMD_PHY)		+= amd.o
 obj-$(CONFIG_AQUANTIA_PHY)	+= aquantia/
-obj-$(CONFIG_AT803X_PHY)	+= at803x.o
+obj-$(CONFIG_AT803X_PHY)	+= qcom/
 obj-$(CONFIG_AX88796B_PHY)	+= ax88796b.o
 obj-$(CONFIG_BCM54140_PHY)	+= bcm54140.o
 obj-$(CONFIG_BCM63XX_PHY)	+= bcm63xx.o
diff --git a/drivers/net/phy/qcom/Kconfig b/drivers/net/phy/qcom/Kconfig
new file mode 100644
index 000000000000..2c274fbbe410
--- /dev/null
+++ b/drivers/net/phy/qcom/Kconfig
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config AT803X_PHY
+	tristate "Qualcomm Atheros AR803X PHYs and QCA833x PHYs"
+	depends on REGULATOR
+	help
+	  Currently supports the AR8030, AR8031, AR8033, AR8035 and internal
+	  QCA8337(Internal qca8k PHY) model
diff --git a/drivers/net/phy/qcom/Makefile b/drivers/net/phy/qcom/Makefile
new file mode 100644
index 000000000000..6a68da8aaa7b
--- /dev/null
+++ b/drivers/net/phy/qcom/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_AT803X_PHY)	+= at803x.o
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/qcom/at803x.c
similarity index 100%
rename from drivers/net/phy/at803x.c
rename to drivers/net/phy/qcom/at803x.c
-- 
2.40.1

[net-next PATCH 13/14] net: phy: qcom: deatch qca83xx PHY driver from at803x

[Christian Marangi]

Deatch qca83xx PHY driver from at803x.

The QCA83xx PHYs implement specific function and doesn't use generic
at803x so it can be detached from the driver and moved to a dedicated
one.

Some function for debug reg access are shared between the new qca83xx
driver and the at803x driver, for this reason some function are also
detached from at803x and moved to a common .c to be used by the 2
driver.

This is to make slimmer PHY drivers instead of including lots of bloat
that would never be used in specific SoC.

A new Kconfig flag QCA83XX_PHY is introduced to compile the new
introduced PHY driver.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/qcom/Kconfig   |   7 +-
 drivers/net/phy/qcom/Makefile  |   3 +-
 drivers/net/phy/qcom/at803x.c  | 326 +--------------------------------
 drivers/net/phy/qcom/common.c  |  50 +++++
 drivers/net/phy/qcom/qca83xx.c | 275 +++++++++++++++++++++++++++
 drivers/net/phy/qcom/qcom.h    |  38 ++++
 6 files changed, 374 insertions(+), 325 deletions(-)
 create mode 100644 drivers/net/phy/qcom/common.c
 create mode 100644 drivers/net/phy/qcom/qca83xx.c
 create mode 100644 drivers/net/phy/qcom/qcom.h

diff --git a/drivers/net/phy/qcom/Kconfig b/drivers/net/phy/qcom/Kconfig
index 2c274fbbe410..06914f23a92e 100644
--- a/drivers/net/phy/qcom/Kconfig
+++ b/drivers/net/phy/qcom/Kconfig
@@ -1,7 +1,12 @@
 # SPDX-License-Identifier: GPL-2.0-only
 config AT803X_PHY
-	tristate "Qualcomm Atheros AR803X PHYs and QCA833x PHYs"
+	tristate "Qualcomm Atheros AR803X PHYs"
 	depends on REGULATOR
 	help
 	  Currently supports the AR8030, AR8031, AR8033, AR8035 and internal
 	  QCA8337(Internal qca8k PHY) model
+
+config QCA83XX_PHY
+	tristate "Qualcomm Atheros QCA833x PHYs"
+	help
+	  Currently supports the internal QCA8337(Internal qca8k PHY) model
diff --git a/drivers/net/phy/qcom/Makefile b/drivers/net/phy/qcom/Makefile
index 6a68da8aaa7b..43e4d14df8ea 100644
--- a/drivers/net/phy/qcom/Makefile
+++ b/drivers/net/phy/qcom/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_AT803X_PHY)	+= at803x.o
+obj-$(CONFIG_AT803X_PHY)	+= at803x.o common.o
+obj-$(CONFIG_QCA83XX_PHY)	+= qca83xx.o common.o
diff --git a/drivers/net/phy/qcom/at803x.c b/drivers/net/phy/qcom/at803x.c
index c6aa31495324..80da66c3d3e9 100644
--- a/drivers/net/phy/qcom/at803x.c
+++ b/drivers/net/phy/qcom/at803x.c
@@ -22,6 +22,8 @@
 #include <linux/sfp.h>
 #include <dt-bindings/net/qca-ar803x.h>
 
+#include "qcom.h"
+
 #define AT803X_SPECIFIC_FUNCTION_CONTROL	0x10
 #define AT803X_SFC_ASSERT_CRS			BIT(11)
 #define AT803X_SFC_FORCE_LINK			BIT(10)
@@ -84,9 +86,6 @@
 #define AT803X_REG_CHIP_CONFIG			0x1f
 #define AT803X_BT_BX_REG_SEL			0x8000
 
-#define AT803X_DEBUG_ADDR			0x1D
-#define AT803X_DEBUG_DATA			0x1E
-
 #define AT803X_MODE_CFG_MASK			0x0F
 #define AT803X_MODE_CFG_BASET_RGMII		0x00
 #define AT803X_MODE_CFG_BASET_SGMII		0x01
@@ -103,30 +102,6 @@
 #define AT803X_PSSR				0x11	/*PHY-Specific Status Register*/
 #define AT803X_PSSR_MR_AN_COMPLETE		0x0200
 
-#define AT803X_DEBUG_ANALOG_TEST_CTRL		0x00
-#define QCA8327_DEBUG_MANU_CTRL_EN		BIT(2)
-#define QCA8337_DEBUG_MANU_CTRL_EN		GENMASK(3, 2)
-#define AT803X_DEBUG_RX_CLK_DLY_EN		BIT(15)
-
-#define AT803X_DEBUG_SYSTEM_CTRL_MODE		0x05
-#define AT803X_DEBUG_TX_CLK_DLY_EN		BIT(8)
-
-#define AT803X_DEBUG_REG_HIB_CTRL		0x0b
-#define   AT803X_DEBUG_HIB_CTRL_SEL_RST_80U	BIT(10)
-#define   AT803X_DEBUG_HIB_CTRL_EN_ANY_CHANGE	BIT(13)
-#define   AT803X_DEBUG_HIB_CTRL_PS_HIB_EN	BIT(15)
-
-#define AT803X_DEBUG_REG_3C			0x3C
-
-#define AT803X_DEBUG_REG_GREEN			0x3D
-#define   AT803X_DEBUG_GATE_CLK_IN1000		BIT(6)
-
-#define AT803X_DEBUG_REG_1F			0x1F
-#define AT803X_DEBUG_PLL_ON			BIT(2)
-#define AT803X_DEBUG_RGMII_1V8			BIT(3)
-
-#define MDIO_AZ_DEBUG				0x800D
-
 /* AT803x supports either the XTAL input pad, an internal PLL or the
  * DSP as clock reference for the clock output pad. The XTAL reference
  * is only used for 25 MHz output, all other frequencies need the PLL.
@@ -177,17 +152,12 @@
 
 #define QCA8081_PHY_ID				0x004dd101
 
-#define QCA8327_A_PHY_ID			0x004dd033
-#define QCA8327_B_PHY_ID			0x004dd034
-#define QCA8337_PHY_ID				0x004dd036
 #define QCA9561_PHY_ID				0x004dd042
-#define QCA8K_PHY_ID_MASK			0xffffffff
-
-#define QCA8K_DEVFLAGS_REVISION_MASK		GENMASK(2, 0)
 
 #define AT803X_PAGE_FIBER			0
 #define AT803X_PAGE_COPPER			1
 
+/* AT803x priv flags */
 /* don't turn off internal PLL */
 #define AT803X_KEEP_PLL_ENABLED			BIT(0)
 #define AT803X_DISABLE_SMARTEEE			BIT(1)
@@ -283,33 +253,11 @@ MODULE_DESCRIPTION("Qualcomm Atheros AR803x and QCA808X PHY driver");
 MODULE_AUTHOR("Matus Ujhelyi");
 MODULE_LICENSE("GPL");
 
-enum stat_access_type {
-	PHY,
-	MMD
-};
-
-struct at803x_hw_stat {
-	const char *string;
-	u8 reg;
-	u32 mask;
-	enum stat_access_type access_type;
-};
-
-static struct at803x_hw_stat qca83xx_hw_stats[] = {
-	{ "phy_idle_errors", 0xa, GENMASK(7, 0), PHY},
-	{ "phy_receive_errors", 0x15, GENMASK(15, 0), PHY},
-	{ "eee_wake_errors", 0x16, GENMASK(15, 0), MMD},
-};
-
 struct at803x_ss_mask {
 	u16 speed_mask;
 	u8 speed_shift;
 };
 
-struct qca83xx_priv {
-	u64 stats[ARRAY_SIZE(qca83xx_hw_stats)];
-};
-
 struct at8031_data {
 	bool is_fiber;
 	bool is_1000basex;
@@ -337,45 +285,6 @@ struct at803x_context {
 	u16 led_control;
 };
 
-static int at803x_debug_reg_write(struct phy_device *phydev, u16 reg, u16 data)
-{
-	int ret;
-
-	ret = phy_write(phydev, AT803X_DEBUG_ADDR, reg);
-	if (ret < 0)
-		return ret;
-
-	return phy_write(phydev, AT803X_DEBUG_DATA, data);
-}
-
-static int at803x_debug_reg_read(struct phy_device *phydev, u16 reg)
-{
-	int ret;
-
-	ret = phy_write(phydev, AT803X_DEBUG_ADDR, reg);
-	if (ret < 0)
-		return ret;
-
-	return phy_read(phydev, AT803X_DEBUG_DATA);
-}
-
-static int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
-				 u16 clear, u16 set)
-{
-	u16 val;
-	int ret;
-
-	ret = at803x_debug_reg_read(phydev, reg);
-	if (ret < 0)
-		return ret;
-
-	val = ret & 0xffff;
-	val &= ~clear;
-	val |= set;
-
-	return phy_write(phydev, AT803X_DEBUG_DATA, val);
-}
-
 static int at803x_write_page(struct phy_device *phydev, int page)
 {
 	int mask;
@@ -527,53 +436,6 @@ static void at803x_get_wol(struct phy_device *phydev,
 		wol->wolopts |= WAKE_MAGIC;
 }
 
-static int qca83xx_get_sset_count(struct phy_device *phydev)
-{
-	return ARRAY_SIZE(qca83xx_hw_stats);
-}
-
-static void qca83xx_get_strings(struct phy_device *phydev, u8 *data)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(qca83xx_hw_stats); i++) {
-		strscpy(data + i * ETH_GSTRING_LEN,
-			qca83xx_hw_stats[i].string, ETH_GSTRING_LEN);
-	}
-}
-
-static u64 qca83xx_get_stat(struct phy_device *phydev, int i)
-{
-	struct at803x_hw_stat stat = qca83xx_hw_stats[i];
-	struct qca83xx_priv *priv = phydev->priv;
-	int val;
-	u64 ret;
-
-	if (stat.access_type == MMD)
-		val = phy_read_mmd(phydev, MDIO_MMD_PCS, stat.reg);
-	else
-		val = phy_read(phydev, stat.reg);
-
-	if (val < 0) {
-		ret = U64_MAX;
-	} else {
-		val = val & stat.mask;
-		priv->stats[i] += val;
-		ret = priv->stats[i];
-	}
-
-	return ret;
-}
-
-static void qca83xx_get_stats(struct phy_device *phydev,
-			      struct ethtool_stats *stats, u64 *data)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(qca83xx_hw_stats); i++)
-		data[i] = qca83xx_get_stat(phydev, i);
-}
-
 static int at803x_suspend(struct phy_device *phydev)
 {
 	int value;
@@ -1689,138 +1551,6 @@ static int at8035_probe(struct phy_device *phydev)
 	return at8035_parse_dt(phydev);
 }
 
-static int qca83xx_probe(struct phy_device *phydev)
-{
-	struct device *dev = &phydev->mdio.dev;
-	struct qca83xx_priv *priv;
-
-	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-
-	phydev->priv = priv;
-
-	return 0;
-}
-
-static int qca83xx_config_init(struct phy_device *phydev)
-{
-	u8 switch_revision;
-
-	switch_revision = phydev->dev_flags & QCA8K_DEVFLAGS_REVISION_MASK;
-
-	switch (switch_revision) {
-	case 1:
-		/* For 100M waveform */
-		at803x_debug_reg_write(phydev, AT803X_DEBUG_ANALOG_TEST_CTRL, 0x02ea);
-		/* Turn on Gigabit clock */
-		at803x_debug_reg_write(phydev, AT803X_DEBUG_REG_GREEN, 0x68a0);
-		break;
-
-	case 2:
-		phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, 0x0);
-		fallthrough;
-	case 4:
-		phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_AZ_DEBUG, 0x803f);
-		at803x_debug_reg_write(phydev, AT803X_DEBUG_REG_GREEN, 0x6860);
-		at803x_debug_reg_write(phydev, AT803X_DEBUG_SYSTEM_CTRL_MODE, 0x2c46);
-		at803x_debug_reg_write(phydev, AT803X_DEBUG_REG_3C, 0x6000);
-		break;
-	}
-
-	/* Following original QCA sourcecode set port to prefer master */
-	phy_set_bits(phydev, MII_CTRL1000, CTL1000_PREFER_MASTER);
-
-	return 0;
-}
-
-static int qca8327_config_init(struct phy_device *phydev)
-{
-	/* QCA8327 require DAC amplitude adjustment for 100m set to +6%.
-	 * Disable on init and enable only with 100m speed following
-	 * qca original source code.
-	 */
-	at803x_debug_reg_mask(phydev, AT803X_DEBUG_ANALOG_TEST_CTRL,
-			      QCA8327_DEBUG_MANU_CTRL_EN, 0);
-
-	return qca83xx_config_init(phydev);
-}
-
-static void qca83xx_link_change_notify(struct phy_device *phydev)
-{
-	/* Set DAC Amplitude adjustment to +6% for 100m on link running */
-	if (phydev->state == PHY_RUNNING) {
-		if (phydev->speed == SPEED_100)
-			at803x_debug_reg_mask(phydev, AT803X_DEBUG_ANALOG_TEST_CTRL,
-					      QCA8327_DEBUG_MANU_CTRL_EN,
-					      QCA8327_DEBUG_MANU_CTRL_EN);
-	} else {
-		/* Reset DAC Amplitude adjustment */
-		at803x_debug_reg_mask(phydev, AT803X_DEBUG_ANALOG_TEST_CTRL,
-				      QCA8327_DEBUG_MANU_CTRL_EN, 0);
-	}
-}
-
-static int qca83xx_resume(struct phy_device *phydev)
-{
-	int ret, val;
-
-	/* Skip reset if not suspended */
-	if (!phydev->suspended)
-		return 0;
-
-	/* Reinit the port, reset values set by suspend */
-	qca83xx_config_init(phydev);
-
-	/* Reset the port on port resume */
-	phy_set_bits(phydev, MII_BMCR, BMCR_RESET | BMCR_ANENABLE);
-
-	/* On resume from suspend the switch execute a reset and
-	 * restart auto-negotiation. Wait for reset to complete.
-	 */
-	ret = phy_read_poll_timeout(phydev, MII_BMCR, val, !(val & BMCR_RESET),
-				    50000, 600000, true);
-	if (ret)
-		return ret;
-
-	msleep(1);
-
-	return 0;
-}
-
-static int qca83xx_suspend(struct phy_device *phydev)
-{
-	at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_GREEN,
-			      AT803X_DEBUG_GATE_CLK_IN1000, 0);
-
-	at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,
-			      AT803X_DEBUG_HIB_CTRL_EN_ANY_CHANGE |
-			      AT803X_DEBUG_HIB_CTRL_SEL_RST_80U, 0);
-
-	return 0;
-}
-
-static int qca8337_suspend(struct phy_device *phydev)
-{
-	/* Only QCA8337 support actual suspend. */
-	genphy_suspend(phydev);
-
-	return qca83xx_suspend(phydev);
-}
-
-static int qca8327_suspend(struct phy_device *phydev)
-{
-	u16 mask = 0;
-
-	/* QCA8327 cause port unreliability when phy suspend
-	 * is set.
-	 */
-	mask |= ~(BMCR_SPEED1000 | BMCR_FULLDPLX);
-	phy_modify(phydev, MII_BMCR, mask, 0);
-
-	return qca83xx_suspend(phydev);
-}
-
 static int qca808x_config_aneg(struct phy_device *phydev)
 {
 	int phy_ctrl = 0;
@@ -2345,53 +2075,6 @@ static struct phy_driver at803x_driver[] = {
 	.read_status		= at803x_read_status,
 	.soft_reset		= genphy_soft_reset,
 	.config_aneg		= at803x_config_aneg,
-}, {
-	/* QCA8337 */
-	.phy_id			= QCA8337_PHY_ID,
-	.phy_id_mask		= QCA8K_PHY_ID_MASK,
-	.name			= "Qualcomm Atheros 8337 internal PHY",
-	/* PHY_GBIT_FEATURES */
-	.probe			= qca83xx_probe,
-	.flags			= PHY_IS_INTERNAL,
-	.config_init		= qca83xx_config_init,
-	.soft_reset		= genphy_soft_reset,
-	.get_sset_count		= qca83xx_get_sset_count,
-	.get_strings		= qca83xx_get_strings,
-	.get_stats		= qca83xx_get_stats,
-	.suspend		= qca8337_suspend,
-	.resume			= qca83xx_resume,
-}, {
-	/* QCA8327-A from switch QCA8327-AL1A */
-	.phy_id			= QCA8327_A_PHY_ID,
-	.phy_id_mask		= QCA8K_PHY_ID_MASK,
-	.name			= "Qualcomm Atheros 8327-A internal PHY",
-	/* PHY_GBIT_FEATURES */
-	.link_change_notify	= qca83xx_link_change_notify,
-	.probe			= qca83xx_probe,
-	.flags			= PHY_IS_INTERNAL,
-	.config_init		= qca8327_config_init,
-	.soft_reset		= genphy_soft_reset,
-	.get_sset_count		= qca83xx_get_sset_count,
-	.get_strings		= qca83xx_get_strings,
-	.get_stats		= qca83xx_get_stats,
-	.suspend		= qca8327_suspend,
-	.resume			= qca83xx_resume,
-}, {
-	/* QCA8327-B from switch QCA8327-BL1A */
-	.phy_id			= QCA8327_B_PHY_ID,
-	.phy_id_mask		= QCA8K_PHY_ID_MASK,
-	.name			= "Qualcomm Atheros 8327-B internal PHY",
-	/* PHY_GBIT_FEATURES */
-	.link_change_notify	= qca83xx_link_change_notify,
-	.probe			= qca83xx_probe,
-	.flags			= PHY_IS_INTERNAL,
-	.config_init		= qca8327_config_init,
-	.soft_reset		= genphy_soft_reset,
-	.get_sset_count		= qca83xx_get_sset_count,
-	.get_strings		= qca83xx_get_strings,
-	.get_stats		= qca83xx_get_stats,
-	.suspend		= qca8327_suspend,
-	.resume			= qca83xx_resume,
 }, {
 	/* Qualcomm QCA8081 */
 	PHY_ID_MATCH_EXACT(QCA8081_PHY_ID),
@@ -2423,9 +2106,6 @@ static struct mdio_device_id __maybe_unused atheros_tbl[] = {
 	{ PHY_ID_MATCH_EXACT(ATH8032_PHY_ID) },
 	{ PHY_ID_MATCH_EXACT(ATH8035_PHY_ID) },
 	{ PHY_ID_MATCH_EXACT(ATH9331_PHY_ID) },
-	{ PHY_ID_MATCH_EXACT(QCA8337_PHY_ID) },
-	{ PHY_ID_MATCH_EXACT(QCA8327_A_PHY_ID) },
-	{ PHY_ID_MATCH_EXACT(QCA8327_B_PHY_ID) },
 	{ PHY_ID_MATCH_EXACT(QCA9561_PHY_ID) },
 	{ PHY_ID_MATCH_EXACT(QCA8081_PHY_ID) },
 	{ }
diff --git a/drivers/net/phy/qcom/common.c b/drivers/net/phy/qcom/common.c
new file mode 100644
index 000000000000..1d9b80fea2e9
--- /dev/null
+++ b/drivers/net/phy/qcom/common.c
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/phy.h>
+#include <linux/module.h>
+
+#include <linux/regulator/driver.h>
+#include <linux/of.h>
+#include <linux/phylink.h>
+#include <linux/sfp.h>
+
+#include "qcom.h"
+
+int at803x_debug_reg_read(struct phy_device *phydev, u16 reg)
+{
+	int ret;
+
+	ret = phy_write(phydev, AT803X_DEBUG_ADDR, reg);
+	if (ret < 0)
+		return ret;
+
+	return phy_read(phydev, AT803X_DEBUG_DATA);
+}
+
+int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
+			  u16 clear, u16 set)
+{
+	u16 val;
+	int ret;
+
+	ret = at803x_debug_reg_read(phydev, reg);
+	if (ret < 0)
+		return ret;
+
+	val = ret & 0xffff;
+	val &= ~clear;
+	val |= set;
+
+	return phy_write(phydev, AT803X_DEBUG_DATA, val);
+}
+
+int at803x_debug_reg_write(struct phy_device *phydev, u16 reg, u16 data)
+{
+	int ret;
+
+	ret = phy_write(phydev, AT803X_DEBUG_ADDR, reg);
+	if (ret < 0)
+		return ret;
+
+	return phy_write(phydev, AT803X_DEBUG_DATA, data);
+}
diff --git a/drivers/net/phy/qcom/qca83xx.c b/drivers/net/phy/qcom/qca83xx.c
new file mode 100644
index 000000000000..e0f849d405e6
--- /dev/null
+++ b/drivers/net/phy/qcom/qca83xx.c
@@ -0,0 +1,275 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/phy.h>
+#include <linux/module.h>
+
+#include "qcom.h"
+
+#define AT803X_DEBUG_REG_3C			0x3C
+
+#define AT803X_DEBUG_REG_GREEN			0x3D
+#define   AT803X_DEBUG_GATE_CLK_IN1000		BIT(6)
+
+#define MDIO_AZ_DEBUG				0x800D
+
+#define QCA8327_A_PHY_ID			0x004dd033
+#define QCA8327_B_PHY_ID			0x004dd034
+#define QCA8337_PHY_ID				0x004dd036
+#define QCA8K_PHY_ID_MASK			0xffffffff
+
+#define QCA8K_DEVFLAGS_REVISION_MASK		GENMASK(2, 0)
+
+static struct at803x_hw_stat qca83xx_hw_stats[] = {
+	{ "phy_idle_errors", 0xa, GENMASK(7, 0), PHY},
+	{ "phy_receive_errors", 0x15, GENMASK(15, 0), PHY},
+	{ "eee_wake_errors", 0x16, GENMASK(15, 0), MMD},
+};
+
+struct qca83xx_priv {
+	u64 stats[ARRAY_SIZE(qca83xx_hw_stats)];
+};
+
+MODULE_DESCRIPTION("Qualcomm Atheros QCA83XX PHY driver");
+MODULE_AUTHOR("Matus Ujhelyi");
+MODULE_AUTHOR("Christian Marangi <ansuelsmth@gmail.com>");
+MODULE_LICENSE("GPL");
+
+static int qca83xx_get_sset_count(struct phy_device *phydev)
+{
+	return ARRAY_SIZE(qca83xx_hw_stats);
+}
+
+static void qca83xx_get_strings(struct phy_device *phydev, u8 *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(qca83xx_hw_stats); i++) {
+		strscpy(data + i * ETH_GSTRING_LEN,
+			qca83xx_hw_stats[i].string, ETH_GSTRING_LEN);
+	}
+}
+
+static u64 qca83xx_get_stat(struct phy_device *phydev, int i)
+{
+	struct at803x_hw_stat stat = qca83xx_hw_stats[i];
+	struct qca83xx_priv *priv = phydev->priv;
+	int val;
+	u64 ret;
+
+	if (stat.access_type == MMD)
+		val = phy_read_mmd(phydev, MDIO_MMD_PCS, stat.reg);
+	else
+		val = phy_read(phydev, stat.reg);
+
+	if (val < 0) {
+		ret = U64_MAX;
+	} else {
+		val = val & stat.mask;
+		priv->stats[i] += val;
+		ret = priv->stats[i];
+	}
+
+	return ret;
+}
+
+static void qca83xx_get_stats(struct phy_device *phydev,
+			      struct ethtool_stats *stats, u64 *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(qca83xx_hw_stats); i++)
+		data[i] = qca83xx_get_stat(phydev, i);
+}
+
+static int qca83xx_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct qca83xx_priv *priv;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	phydev->priv = priv;
+
+	return 0;
+}
+
+static int qca83xx_config_init(struct phy_device *phydev)
+{
+	u8 switch_revision;
+
+	switch_revision = phydev->dev_flags & QCA8K_DEVFLAGS_REVISION_MASK;
+
+	switch (switch_revision) {
+	case 1:
+		/* For 100M waveform */
+		at803x_debug_reg_write(phydev, AT803X_DEBUG_ANALOG_TEST_CTRL, 0x02ea);
+		/* Turn on Gigabit clock */
+		at803x_debug_reg_write(phydev, AT803X_DEBUG_REG_GREEN, 0x68a0);
+		break;
+
+	case 2:
+		phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, 0x0);
+		fallthrough;
+	case 4:
+		phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_AZ_DEBUG, 0x803f);
+		at803x_debug_reg_write(phydev, AT803X_DEBUG_REG_GREEN, 0x6860);
+		at803x_debug_reg_write(phydev, AT803X_DEBUG_SYSTEM_CTRL_MODE, 0x2c46);
+		at803x_debug_reg_write(phydev, AT803X_DEBUG_REG_3C, 0x6000);
+		break;
+	}
+
+	/* Following original QCA sourcecode set port to prefer master */
+	phy_set_bits(phydev, MII_CTRL1000, CTL1000_PREFER_MASTER);
+
+	return 0;
+}
+
+static int qca8327_config_init(struct phy_device *phydev)
+{
+	/* QCA8327 require DAC amplitude adjustment for 100m set to +6%.
+	 * Disable on init and enable only with 100m speed following
+	 * qca original source code.
+	 */
+	at803x_debug_reg_mask(phydev, AT803X_DEBUG_ANALOG_TEST_CTRL,
+			      QCA8327_DEBUG_MANU_CTRL_EN, 0);
+
+	return qca83xx_config_init(phydev);
+}
+
+static void qca83xx_link_change_notify(struct phy_device *phydev)
+{
+	/* Set DAC Amplitude adjustment to +6% for 100m on link running */
+	if (phydev->state == PHY_RUNNING) {
+		if (phydev->speed == SPEED_100)
+			at803x_debug_reg_mask(phydev, AT803X_DEBUG_ANALOG_TEST_CTRL,
+					      QCA8327_DEBUG_MANU_CTRL_EN,
+					      QCA8327_DEBUG_MANU_CTRL_EN);
+	} else {
+		/* Reset DAC Amplitude adjustment */
+		at803x_debug_reg_mask(phydev, AT803X_DEBUG_ANALOG_TEST_CTRL,
+				      QCA8327_DEBUG_MANU_CTRL_EN, 0);
+	}
+}
+
+static int qca83xx_resume(struct phy_device *phydev)
+{
+	int ret, val;
+
+	/* Skip reset if not suspended */
+	if (!phydev->suspended)
+		return 0;
+
+	/* Reinit the port, reset values set by suspend */
+	qca83xx_config_init(phydev);
+
+	/* Reset the port on port resume */
+	phy_set_bits(phydev, MII_BMCR, BMCR_RESET | BMCR_ANENABLE);
+
+	/* On resume from suspend the switch execute a reset and
+	 * restart auto-negotiation. Wait for reset to complete.
+	 */
+	ret = phy_read_poll_timeout(phydev, MII_BMCR, val, !(val & BMCR_RESET),
+				    50000, 600000, true);
+	if (ret)
+		return ret;
+
+	msleep(1);
+
+	return 0;
+}
+
+static int qca83xx_suspend(struct phy_device *phydev)
+{
+	at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_GREEN,
+			      AT803X_DEBUG_GATE_CLK_IN1000, 0);
+
+	at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,
+			      AT803X_DEBUG_HIB_CTRL_EN_ANY_CHANGE |
+			      AT803X_DEBUG_HIB_CTRL_SEL_RST_80U, 0);
+
+	return 0;
+}
+
+static int qca8337_suspend(struct phy_device *phydev)
+{
+	/* Only QCA8337 support actual suspend. */
+	genphy_suspend(phydev);
+
+	return qca83xx_suspend(phydev);
+}
+
+static int qca8327_suspend(struct phy_device *phydev)
+{
+	u16 mask = 0;
+
+	/* QCA8327 cause port unreliability when phy suspend
+	 * is set.
+	 */
+	mask |= ~(BMCR_SPEED1000 | BMCR_FULLDPLX);
+	phy_modify(phydev, MII_BMCR, mask, 0);
+
+	return qca83xx_suspend(phydev);
+}
+
+static struct phy_driver qca833x_driver[] = {
+{
+	/* QCA8337 */
+	.phy_id			= QCA8337_PHY_ID,
+	.phy_id_mask		= QCA8K_PHY_ID_MASK,
+	.name			= "Qualcomm Atheros 8337 internal PHY",
+	/* PHY_GBIT_FEATURES */
+	.probe			= qca83xx_probe,
+	.flags			= PHY_IS_INTERNAL,
+	.config_init		= qca83xx_config_init,
+	.soft_reset		= genphy_soft_reset,
+	.get_sset_count		= qca83xx_get_sset_count,
+	.get_strings		= qca83xx_get_strings,
+	.get_stats		= qca83xx_get_stats,
+	.suspend		= qca8337_suspend,
+	.resume			= qca83xx_resume,
+}, {
+	/* QCA8327-A from switch QCA8327-AL1A */
+	.phy_id			= QCA8327_A_PHY_ID,
+	.phy_id_mask		= QCA8K_PHY_ID_MASK,
+	.name			= "Qualcomm Atheros 8327-A internal PHY",
+	/* PHY_GBIT_FEATURES */
+	.link_change_notify	= qca83xx_link_change_notify,
+	.probe			= qca83xx_probe,
+	.flags			= PHY_IS_INTERNAL,
+	.config_init		= qca8327_config_init,
+	.soft_reset		= genphy_soft_reset,
+	.get_sset_count		= qca83xx_get_sset_count,
+	.get_strings		= qca83xx_get_strings,
+	.get_stats		= qca83xx_get_stats,
+	.suspend		= qca8327_suspend,
+	.resume			= qca83xx_resume,
+}, {
+	/* QCA8327-B from switch QCA8327-BL1A */
+	.phy_id			= QCA8327_B_PHY_ID,
+	.phy_id_mask		= QCA8K_PHY_ID_MASK,
+	.name			= "Qualcomm Atheros 8327-B internal PHY",
+	/* PHY_GBIT_FEATURES */
+	.link_change_notify	= qca83xx_link_change_notify,
+	.probe			= qca83xx_probe,
+	.flags			= PHY_IS_INTERNAL,
+	.config_init		= qca8327_config_init,
+	.soft_reset		= genphy_soft_reset,
+	.get_sset_count		= qca83xx_get_sset_count,
+	.get_strings		= qca83xx_get_strings,
+	.get_stats		= qca83xx_get_stats,
+	.suspend		= qca8327_suspend,
+	.resume			= qca83xx_resume,
+}, };
+
+module_phy_driver(qca833x_driver);
+
+static struct mdio_device_id __maybe_unused qca83xx_tbl[] = {
+	{ PHY_ID_MATCH_EXACT(QCA8337_PHY_ID) },
+	{ PHY_ID_MATCH_EXACT(QCA8327_A_PHY_ID) },
+	{ PHY_ID_MATCH_EXACT(QCA8327_B_PHY_ID) },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(mdio, qca83xx_tbl);
diff --git a/drivers/net/phy/qcom/qcom.h b/drivers/net/phy/qcom/qcom.h
new file mode 100644
index 000000000000..17cc6705dd3e
--- /dev/null
+++ b/drivers/net/phy/qcom/qcom.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#define AT803X_DEBUG_ANALOG_TEST_CTRL		0x00
+#define QCA8327_DEBUG_MANU_CTRL_EN		BIT(2)
+#define QCA8337_DEBUG_MANU_CTRL_EN		GENMASK(3, 2)
+#define AT803X_DEBUG_RX_CLK_DLY_EN		BIT(15)
+
+#define AT803X_DEBUG_SYSTEM_CTRL_MODE		0x05
+#define AT803X_DEBUG_TX_CLK_DLY_EN		BIT(8)
+
+#define AT803X_DEBUG_REG_HIB_CTRL		0x0b
+#define   AT803X_DEBUG_HIB_CTRL_SEL_RST_80U	BIT(10)
+#define   AT803X_DEBUG_HIB_CTRL_EN_ANY_CHANGE	BIT(13)
+#define   AT803X_DEBUG_HIB_CTRL_PS_HIB_EN	BIT(15)
+
+#define AT803X_DEBUG_ADDR			0x1D
+#define AT803X_DEBUG_DATA			0x1E
+
+#define AT803X_DEBUG_REG_1F			0x1F
+#define AT803X_DEBUG_PLL_ON			BIT(2)
+#define AT803X_DEBUG_RGMII_1V8			BIT(3)
+
+enum stat_access_type {
+	PHY,
+	MMD
+};
+
+struct at803x_hw_stat {
+	const char *string;
+	u8 reg;
+	u32 mask;
+	enum stat_access_type access_type;
+};
+
+int at803x_debug_reg_read(struct phy_device *phydev, u16 reg);
+int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
+			  u16 clear, u16 set);
+int at803x_debug_reg_write(struct phy_device *phydev, u16 reg, u16 data);
-- 
2.40.1

Re: [net-next PATCH 13/14] net: phy: qcom: deatch qca83xx PHY driver from at803x

[Russell King (Oracle)]

On Wed, Nov 29, 2023 at 03:12:18AM +0100, Christian Marangi wrote:
> diff --git a/drivers/net/phy/qcom/Makefile b/drivers/net/phy/qcom/Makefile
> index 6a68da8aaa7b..43e4d14df8ea 100644
> --- a/drivers/net/phy/qcom/Makefile
> +++ b/drivers/net/phy/qcom/Makefile
> @@ -1,2 +1,3 @@
>  # SPDX-License-Identifier: GPL-2.0
> -obj-$(CONFIG_AT803X_PHY)	+= at803x.o
> +obj-$(CONFIG_AT803X_PHY)	+= at803x.o common.o
> +obj-$(CONFIG_QCA83XX_PHY)	+= qca83xx.o common.o

These PHY drivers can be built as modules. You will end up with several
modules - at803x.ko, qca83xx.ko and common.ko. You don't mark any
functions in common.c as exported, no module license, no author, no
description. common.ko is way too generic a name as well.

Please think about this more and test building these drivers as a
module.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [net-next PATCH 13/14] net: phy: qcom: deatch qca83xx PHY driver from at803x

[Christian Marangi]

On Wed, Nov 29, 2023 at 09:53:00AM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 29, 2023 at 03:12:18AM +0100, Christian Marangi wrote:
> > diff --git a/drivers/net/phy/qcom/Makefile b/drivers/net/phy/qcom/Makefile
> > index 6a68da8aaa7b..43e4d14df8ea 100644
> > --- a/drivers/net/phy/qcom/Makefile
> > +++ b/drivers/net/phy/qcom/Makefile
> > @@ -1,2 +1,3 @@
> >  # SPDX-License-Identifier: GPL-2.0
> > -obj-$(CONFIG_AT803X_PHY)	+= at803x.o
> > +obj-$(CONFIG_AT803X_PHY)	+= at803x.o common.o
> > +obj-$(CONFIG_QCA83XX_PHY)	+= qca83xx.o common.o
> 
> These PHY drivers can be built as modules. You will end up with several
> modules - at803x.ko, qca83xx.ko and common.ko. You don't mark any
> functions in common.c as exported, no module license, no author, no
> description. common.ko is way too generic a name as well.
> 
> Please think about this more and test building these drivers as a
> module.
>

Had some fear about this...

What would be the preferred way for this?

Having a .ko that EXPORT symbol or making the PHY driver .ko to compile
the common.o in it?

Honestly I would like the second option since I would prefer not to
create a .ko with shared function and EXPORT lots of symbols. On SoC it's
expected to have only one of the PHY (at max 2 when the qca807x PHY will
be implemented, with the at808x also present) so the size increase is
minimal.

(just to be more clear, talking about this makefile implementation)

at803x-objs			+= common.o
obj-$(CONFIG_AT803X_PHY)	+= at803x.o
qca83xx-objs			+= common.o
obj-$(CONFIG_QCA83XX_PHY)	+= qca83xx.o
qca808x-objs			+= common.o
obj-$(CONFIG_QCA808X_PHY)	+= qca808x.o

For name of common.c, is qcom_ethphy_common.c a better name?

-- 
	Ansuel

Re: [net-next PATCH 13/14] net: phy: qcom: deatch qca83xx PHY driver from at803x

[Russell King (Oracle)]

On Wed, Nov 29, 2023 at 11:37:56AM +0100, Christian Marangi wrote:
> On Wed, Nov 29, 2023 at 09:53:00AM +0000, Russell King (Oracle) wrote:
> > On Wed, Nov 29, 2023 at 03:12:18AM +0100, Christian Marangi wrote:
> > > diff --git a/drivers/net/phy/qcom/Makefile b/drivers/net/phy/qcom/Makefile
> > > index 6a68da8aaa7b..43e4d14df8ea 100644
> > > --- a/drivers/net/phy/qcom/Makefile
> > > +++ b/drivers/net/phy/qcom/Makefile
> > > @@ -1,2 +1,3 @@
> > >  # SPDX-License-Identifier: GPL-2.0
> > > -obj-$(CONFIG_AT803X_PHY)	+= at803x.o
> > > +obj-$(CONFIG_AT803X_PHY)	+= at803x.o common.o
> > > +obj-$(CONFIG_QCA83XX_PHY)	+= qca83xx.o common.o
> > 
> > These PHY drivers can be built as modules. You will end up with several
> > modules - at803x.ko, qca83xx.ko and common.ko. You don't mark any
> > functions in common.c as exported, no module license, no author, no
> > description. common.ko is way too generic a name as well.
> > 
> > Please think about this more and test building these drivers as a
> > module.
> >
> 
> Had some fear about this...
> 
> What would be the preferred way for this?
> 
> Having a .ko that EXPORT symbol or making the PHY driver .ko to compile
> the common.o in it?

I think the former, otherwise we end up with common.o duplicated in
each module, which becomes unnecessary bloat. This is how the Broadcom
stuff (which also has a "library") does it.

> Honestly I would like the second option since I would prefer not to
> create a .ko with shared function and EXPORT lots of symbols. On SoC it's
> expected to have only one of the PHY (at max 2 when the qca807x PHY will
> be implemented, with the at808x also present) so the size increase is
> minimal.
> 
> (just to be more clear, talking about this makefile implementation)
> 
> at803x-objs			+= common.o
> obj-$(CONFIG_AT803X_PHY)	+= at803x.o
> qca83xx-objs			+= common.o
> obj-$(CONFIG_QCA83XX_PHY)	+= qca83xx.o
> qca808x-objs			+= common.o
> obj-$(CONFIG_QCA808X_PHY)	+= qca808x.o

That won't work - the -objs needs to list the corresponding .o file
as well, and it needs to be a different name (you can't do this:

qca808x-objs			+= common.o qca808x.o

it has to be something like:

qca808x-phy-objs		+= common.o qca808x.o
obj-$(CONFIG_QCA808X_PHY)	+= qca808x-phy.o

However, I don't like this because it means each module ends up with
a copy of common.o in it.

> For name of common.c, is qcom_ethphy_common.c a better name?

or qcom-phy-lib.c which follows what we have for Broadcom.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [net-next PATCH 13/14] net: phy: qcom: deatch qca83xx PHY driver from at803x

[Christian Marangi]

On Wed, Nov 29, 2023 at 11:20:25AM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 29, 2023 at 11:37:56AM +0100, Christian Marangi wrote:
> > On Wed, Nov 29, 2023 at 09:53:00AM +0000, Russell King (Oracle) wrote:
> > > On Wed, Nov 29, 2023 at 03:12:18AM +0100, Christian Marangi wrote:
> > > > diff --git a/drivers/net/phy/qcom/Makefile b/drivers/net/phy/qcom/Makefile
> > > > index 6a68da8aaa7b..43e4d14df8ea 100644
> > > > --- a/drivers/net/phy/qcom/Makefile
> > > > +++ b/drivers/net/phy/qcom/Makefile
> > > > @@ -1,2 +1,3 @@
> > > >  # SPDX-License-Identifier: GPL-2.0
> > > > -obj-$(CONFIG_AT803X_PHY)	+= at803x.o
> > > > +obj-$(CONFIG_AT803X_PHY)	+= at803x.o common.o
> > > > +obj-$(CONFIG_QCA83XX_PHY)	+= qca83xx.o common.o
> > > 
> > > These PHY drivers can be built as modules. You will end up with several
> > > modules - at803x.ko, qca83xx.ko and common.ko. You don't mark any
> > > functions in common.c as exported, no module license, no author, no
> > > description. common.ko is way too generic a name as well.
> > > 
> > > Please think about this more and test building these drivers as a
> > > module.
> > >
> > 
> > Had some fear about this...
> > 
> > What would be the preferred way for this?
> > 
> > Having a .ko that EXPORT symbol or making the PHY driver .ko to compile
> > the common.o in it?
> 
> I think the former, otherwise we end up with common.o duplicated in
> each module, which becomes unnecessary bloat. This is how the Broadcom
> stuff (which also has a "library") does it.
> 
> > Honestly I would like the second option since I would prefer not to
> > create a .ko with shared function and EXPORT lots of symbols. On SoC it's
> > expected to have only one of the PHY (at max 2 when the qca807x PHY will
> > be implemented, with the at808x also present) so the size increase is
> > minimal.
> > 
> > (just to be more clear, talking about this makefile implementation)
> > 
> > at803x-objs			+= common.o
> > obj-$(CONFIG_AT803X_PHY)	+= at803x.o
> > qca83xx-objs			+= common.o
> > obj-$(CONFIG_QCA83XX_PHY)	+= qca83xx.o
> > qca808x-objs			+= common.o
> > obj-$(CONFIG_QCA808X_PHY)	+= qca808x.o
> 
> That won't work - the -objs needs to list the corresponding .o file
> as well, and it needs to be a different name (you can't do this:
> 
> qca808x-objs			+= common.o qca808x.o
> 
> it has to be something like:
> 
> qca808x-phy-objs		+= common.o qca808x.o
> obj-$(CONFIG_QCA808X_PHY)	+= qca808x-phy.o
> 
> However, I don't like this because it means each module ends up with
> a copy of common.o in it.
> 

Ok will do the .ko library approach np.

> > For name of common.c, is qcom_ethphy_common.c a better name?
> 
> or qcom-phy-lib.c which follows what we have for Broadcom.
>

Think we have to add some reference that it's about ethernet somewhere.

There are already lots of qcom-phy .ko outside net/ 

qcom-ethphy-lib.c ?

-- 
	Ansuel

[net-next PATCH 14/14] net: phy: qcom: detach qca808x PHY driver from at803x

[Christian Marangi]

Almost all the QCA8081 PHY driver OPs are specific and only some of them
use the generic at803x.

To make the at803x code slimmer, move all the specific qca808x regs and
functions to a dedicated PHY driver.

All the shared function are moved to the shared .c since they are both
used by qca808x and at803x.

Also a new Kconfig is introduced QCA808X_PHY, to compile the newly
introduced PHY driver for QCA8081 PHY.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/qcom/Kconfig   |   5 +
 drivers/net/phy/qcom/Makefile  |   1 +
 drivers/net/phy/qcom/at803x.c  | 894 +--------------------------------
 drivers/net/phy/qcom/common.c  | 303 ++++++++++-
 drivers/net/phy/qcom/qca808x.c | 550 ++++++++++++++++++++
 drivers/net/phy/qcom/qcom.h    |  86 ++++
 6 files changed, 945 insertions(+), 894 deletions(-)
 create mode 100644 drivers/net/phy/qcom/qca808x.c

diff --git a/drivers/net/phy/qcom/Kconfig b/drivers/net/phy/qcom/Kconfig
index 06914f23a92e..0c227f2c2917 100644
--- a/drivers/net/phy/qcom/Kconfig
+++ b/drivers/net/phy/qcom/Kconfig
@@ -10,3 +10,8 @@ config QCA83XX_PHY
 	tristate "Qualcomm Atheros QCA833x PHYs"
 	help
 	  Currently supports the internal QCA8337(Internal qca8k PHY) model
+
+config QCA808X_PHY
+	tristate "Qualcomm QCA808x PHYs"
+	help
+	  Currently supports the QCA8081 model
diff --git a/drivers/net/phy/qcom/Makefile b/drivers/net/phy/qcom/Makefile
index 43e4d14df8ea..391c2dde8131 100644
--- a/drivers/net/phy/qcom/Makefile
+++ b/drivers/net/phy/qcom/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_AT803X_PHY)	+= at803x.o common.o
 obj-$(CONFIG_QCA83XX_PHY)	+= qca83xx.o common.o
+obj-$(CONFIG_QCA808X_PHY)	+= qca808x.o common.o
diff --git a/drivers/net/phy/qcom/at803x.c b/drivers/net/phy/qcom/at803x.c
index 80da66c3d3e9..1370b511bd96 100644
--- a/drivers/net/phy/qcom/at803x.c
+++ b/drivers/net/phy/qcom/at803x.c
@@ -24,65 +24,8 @@
 
 #include "qcom.h"
 
-#define AT803X_SPECIFIC_FUNCTION_CONTROL	0x10
-#define AT803X_SFC_ASSERT_CRS			BIT(11)
-#define AT803X_SFC_FORCE_LINK			BIT(10)
-#define AT803X_SFC_MDI_CROSSOVER_MODE_M		GENMASK(6, 5)
-#define AT803X_SFC_AUTOMATIC_CROSSOVER		0x3
-#define AT803X_SFC_MANUAL_MDIX			0x1
-#define AT803X_SFC_MANUAL_MDI			0x0
-#define AT803X_SFC_SQE_TEST			BIT(2)
-#define AT803X_SFC_POLARITY_REVERSAL		BIT(1)
-#define AT803X_SFC_DISABLE_JABBER		BIT(0)
-
-#define AT803X_SPECIFIC_STATUS			0x11
-#define AT803X_SS_SPEED_MASK			GENMASK(15, 14)
-#define AT803X_SS_SPEED_1000			2
-#define AT803X_SS_SPEED_100			1
-#define AT803X_SS_SPEED_10			0
-#define AT803X_SS_DUPLEX			BIT(13)
-#define AT803X_SS_SPEED_DUPLEX_RESOLVED		BIT(11)
-#define AT803X_SS_MDIX				BIT(6)
-
-#define QCA808X_SS_SPEED_MASK			GENMASK(9, 7)
-#define QCA808X_SS_SPEED_2500			4
-
-#define AT803X_INTR_ENABLE			0x12
-#define AT803X_INTR_ENABLE_AUTONEG_ERR		BIT(15)
-#define AT803X_INTR_ENABLE_SPEED_CHANGED	BIT(14)
-#define AT803X_INTR_ENABLE_DUPLEX_CHANGED	BIT(13)
-#define AT803X_INTR_ENABLE_PAGE_RECEIVED	BIT(12)
-#define AT803X_INTR_ENABLE_LINK_FAIL		BIT(11)
-#define AT803X_INTR_ENABLE_LINK_SUCCESS		BIT(10)
-#define AT803X_INTR_ENABLE_LINK_FAIL_BX		BIT(8)
-#define AT803X_INTR_ENABLE_LINK_SUCCESS_BX	BIT(7)
-#define AT803X_INTR_ENABLE_WIRESPEED_DOWNGRADE	BIT(5)
-#define AT803X_INTR_ENABLE_POLARITY_CHANGED	BIT(1)
-#define AT803X_INTR_ENABLE_WOL			BIT(0)
-
-#define AT803X_INTR_STATUS			0x13
-
-#define AT803X_SMART_SPEED			0x14
-#define AT803X_SMART_SPEED_ENABLE		BIT(5)
-#define AT803X_SMART_SPEED_RETRY_LIMIT_MASK	GENMASK(4, 2)
-#define AT803X_SMART_SPEED_BYPASS_TIMER		BIT(1)
-#define AT803X_CDT				0x16
-#define AT803X_CDT_MDI_PAIR_MASK		GENMASK(9, 8)
-#define AT803X_CDT_ENABLE_TEST			BIT(0)
-#define AT803X_CDT_STATUS			0x1c
-#define AT803X_CDT_STATUS_STAT_NORMAL		0
-#define AT803X_CDT_STATUS_STAT_SHORT		1
-#define AT803X_CDT_STATUS_STAT_OPEN		2
-#define AT803X_CDT_STATUS_STAT_FAIL		3
-#define AT803X_CDT_STATUS_STAT_MASK		GENMASK(9, 8)
-#define AT803X_CDT_STATUS_DELTA_TIME_MASK	GENMASK(7, 0)
 #define AT803X_LED_CONTROL			0x18
 
-#define AT803X_PHY_MMD3_WOL_CTRL		0x8012
-#define AT803X_WOL_EN				BIT(5)
-#define AT803X_LOC_MAC_ADDR_0_15_OFFSET		0x804C
-#define AT803X_LOC_MAC_ADDR_16_31_OFFSET	0x804B
-#define AT803X_LOC_MAC_ADDR_32_47_OFFSET	0x804A
 #define AT803X_REG_CHIP_CONFIG			0x1f
 #define AT803X_BT_BX_REG_SEL			0x8000
 
@@ -134,10 +77,6 @@
 #define AT803X_CLK_OUT_STRENGTH_HALF		1
 #define AT803X_CLK_OUT_STRENGTH_QUARTER		2
 
-#define AT803X_DEFAULT_DOWNSHIFT		5
-#define AT803X_MIN_DOWNSHIFT			2
-#define AT803X_MAX_DOWNSHIFT			9
-
 #define AT803X_MMD3_SMARTEEE_CTL1		0x805b
 #define AT803X_MMD3_SMARTEEE_CTL2		0x805c
 #define AT803X_MMD3_SMARTEEE_CTL3		0x805d
@@ -150,8 +89,6 @@
 #define ATH8035_PHY_ID				0x004dd072
 #define AT8030_PHY_ID_MASK			0xffffffef
 
-#define QCA8081_PHY_ID				0x004dd101
-
 #define QCA9561_PHY_ID				0x004dd042
 
 #define AT803X_PAGE_FIBER			0
@@ -165,99 +102,10 @@
 /* disable hibernation mode */
 #define AT803X_DISABLE_HIBERNATION_MODE		BIT(2)
 
-/* ADC threshold */
-#define QCA808X_PHY_DEBUG_ADC_THRESHOLD		0x2c80
-#define QCA808X_ADC_THRESHOLD_MASK		GENMASK(7, 0)
-#define QCA808X_ADC_THRESHOLD_80MV		0
-#define QCA808X_ADC_THRESHOLD_100MV		0xf0
-#define QCA808X_ADC_THRESHOLD_200MV		0x0f
-#define QCA808X_ADC_THRESHOLD_300MV		0xff
-
-/* CLD control */
-#define QCA808X_PHY_MMD3_ADDR_CLD_CTRL7		0x8007
-#define QCA808X_8023AZ_AFE_CTRL_MASK		GENMASK(8, 4)
-#define QCA808X_8023AZ_AFE_EN			0x90
-
-/* AZ control */
-#define QCA808X_PHY_MMD3_AZ_TRAINING_CTRL	0x8008
-#define QCA808X_MMD3_AZ_TRAINING_VAL		0x1c32
-
-#define QCA808X_PHY_MMD1_MSE_THRESHOLD_20DB	0x8014
-#define QCA808X_MSE_THRESHOLD_20DB_VALUE	0x529
-
-#define QCA808X_PHY_MMD1_MSE_THRESHOLD_17DB	0x800E
-#define QCA808X_MSE_THRESHOLD_17DB_VALUE	0x341
-
-#define QCA808X_PHY_MMD1_MSE_THRESHOLD_27DB	0x801E
-#define QCA808X_MSE_THRESHOLD_27DB_VALUE	0x419
-
-#define QCA808X_PHY_MMD1_MSE_THRESHOLD_28DB	0x8020
-#define QCA808X_MSE_THRESHOLD_28DB_VALUE	0x341
-
-#define QCA808X_PHY_MMD7_TOP_OPTION1		0x901c
-#define QCA808X_TOP_OPTION1_DATA		0x0
-
-#define QCA808X_PHY_MMD3_DEBUG_1		0xa100
-#define QCA808X_MMD3_DEBUG_1_VALUE		0x9203
-#define QCA808X_PHY_MMD3_DEBUG_2		0xa101
-#define QCA808X_MMD3_DEBUG_2_VALUE		0x48ad
-#define QCA808X_PHY_MMD3_DEBUG_3		0xa103
-#define QCA808X_MMD3_DEBUG_3_VALUE		0x1698
-#define QCA808X_PHY_MMD3_DEBUG_4		0xa105
-#define QCA808X_MMD3_DEBUG_4_VALUE		0x8001
-#define QCA808X_PHY_MMD3_DEBUG_5		0xa106
-#define QCA808X_MMD3_DEBUG_5_VALUE		0x1111
-#define QCA808X_PHY_MMD3_DEBUG_6		0xa011
-#define QCA808X_MMD3_DEBUG_6_VALUE		0x5f85
-
-/* master/slave seed config */
-#define QCA808X_PHY_DEBUG_LOCAL_SEED		9
-#define QCA808X_MASTER_SLAVE_SEED_ENABLE	BIT(1)
-#define QCA808X_MASTER_SLAVE_SEED_CFG		GENMASK(12, 2)
-#define QCA808X_MASTER_SLAVE_SEED_RANGE		0x32
-
-/* Hibernation yields lower power consumpiton in contrast with normal operation mode.
- * when the copper cable is unplugged, the PHY enters into hibernation mode in about 10s.
- */
-#define QCA808X_DBG_AN_TEST			0xb
-#define QCA808X_HIBERNATION_EN			BIT(15)
-
-#define QCA808X_CDT_ENABLE_TEST			BIT(15)
-#define QCA808X_CDT_INTER_CHECK_DIS		BIT(13)
-#define QCA808X_CDT_LENGTH_UNIT			BIT(10)
-
-#define QCA808X_MMD3_CDT_STATUS			0x8064
-#define QCA808X_MMD3_CDT_DIAG_PAIR_A		0x8065
-#define QCA808X_MMD3_CDT_DIAG_PAIR_B		0x8066
-#define QCA808X_MMD3_CDT_DIAG_PAIR_C		0x8067
-#define QCA808X_MMD3_CDT_DIAG_PAIR_D		0x8068
-#define QCA808X_CDT_DIAG_LENGTH			GENMASK(7, 0)
-
-#define QCA808X_CDT_CODE_PAIR_A			GENMASK(15, 12)
-#define QCA808X_CDT_CODE_PAIR_B			GENMASK(11, 8)
-#define QCA808X_CDT_CODE_PAIR_C			GENMASK(7, 4)
-#define QCA808X_CDT_CODE_PAIR_D			GENMASK(3, 0)
-#define QCA808X_CDT_STATUS_STAT_FAIL		0
-#define QCA808X_CDT_STATUS_STAT_NORMAL		1
-#define QCA808X_CDT_STATUS_STAT_OPEN		2
-#define QCA808X_CDT_STATUS_STAT_SHORT		3
-
-/* QCA808X 1G chip type */
-#define QCA808X_PHY_MMD7_CHIP_TYPE		0x901d
-#define QCA808X_PHY_CHIP_TYPE_1G		BIT(0)
-
-#define QCA8081_PHY_SERDES_MMD1_FIFO_CTRL	0x9072
-#define QCA8081_PHY_FIFO_RSTN			BIT(11)
-
-MODULE_DESCRIPTION("Qualcomm Atheros AR803x and QCA808X PHY driver");
+MODULE_DESCRIPTION("Qualcomm Atheros AR803x PHY driver");
 MODULE_AUTHOR("Matus Ujhelyi");
 MODULE_LICENSE("GPL");
 
-struct at803x_ss_mask {
-	u16 speed_mask;
-	u8 speed_shift;
-};
-
 struct at8031_data {
 	bool is_fiber;
 	bool is_1000basex;
@@ -362,80 +210,6 @@ static void at803x_context_restore(struct phy_device *phydev,
 	phy_write(phydev, AT803X_LED_CONTROL, context->led_control);
 }
 
-static int at803x_set_wol(struct phy_device *phydev,
-			  struct ethtool_wolinfo *wol)
-{
-	int ret, irq_enabled;
-
-	if (wol->wolopts & WAKE_MAGIC) {
-		struct net_device *ndev = phydev->attached_dev;
-		const u8 *mac;
-		unsigned int i;
-		static const unsigned int offsets[] = {
-			AT803X_LOC_MAC_ADDR_32_47_OFFSET,
-			AT803X_LOC_MAC_ADDR_16_31_OFFSET,
-			AT803X_LOC_MAC_ADDR_0_15_OFFSET,
-		};
-
-		if (!ndev)
-			return -ENODEV;
-
-		mac = (const u8 *) ndev->dev_addr;
-
-		if (!is_valid_ether_addr(mac))
-			return -EINVAL;
-
-		for (i = 0; i < 3; i++)
-			phy_write_mmd(phydev, MDIO_MMD_PCS, offsets[i],
-				      mac[(i * 2) + 1] | (mac[(i * 2)] << 8));
-
-		/* Enable WOL interrupt */
-		ret = phy_modify(phydev, AT803X_INTR_ENABLE, 0, AT803X_INTR_ENABLE_WOL);
-		if (ret)
-			return ret;
-	} else {
-		/* Disable WOL interrupt */
-		ret = phy_modify(phydev, AT803X_INTR_ENABLE, AT803X_INTR_ENABLE_WOL, 0);
-		if (ret)
-			return ret;
-	}
-
-	/* Clear WOL status */
-	ret = phy_read(phydev, AT803X_INTR_STATUS);
-	if (ret < 0)
-		return ret;
-
-	/* Check if there are other interrupts except for WOL triggered when PHY is
-	 * in interrupt mode, only the interrupts enabled by AT803X_INTR_ENABLE can
-	 * be passed up to the interrupt PIN.
-	 */
-	irq_enabled = phy_read(phydev, AT803X_INTR_ENABLE);
-	if (irq_enabled < 0)
-		return irq_enabled;
-
-	irq_enabled &= ~AT803X_INTR_ENABLE_WOL;
-	if (ret & irq_enabled && !phy_polling_mode(phydev))
-		phy_trigger_machine(phydev);
-
-	return 0;
-}
-
-static void at803x_get_wol(struct phy_device *phydev,
-			   struct ethtool_wolinfo *wol)
-{
-	int value;
-
-	wol->supported = WAKE_MAGIC;
-	wol->wolopts = 0;
-
-	value = phy_read(phydev, AT803X_INTR_ENABLE);
-	if (value < 0)
-		return;
-
-	if (value & AT803X_INTR_ENABLE_WOL)
-		wol->wolopts |= WAKE_MAGIC;
-}
-
 static int at803x_suspend(struct phy_device *phydev)
 {
 	int value;
@@ -675,73 +449,6 @@ static int at803x_config_init(struct phy_device *phydev)
 	return phy_modify(phydev, MII_ADVERTISE, MDIO_AN_CTRL1_XNP, 0);
 }
 
-static int at803x_ack_interrupt(struct phy_device *phydev)
-{
-	int err;
-
-	err = phy_read(phydev, AT803X_INTR_STATUS);
-
-	return (err < 0) ? err : 0;
-}
-
-static int at803x_config_intr(struct phy_device *phydev)
-{
-	int err;
-	int value;
-
-	value = phy_read(phydev, AT803X_INTR_ENABLE);
-
-	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
-		/* Clear any pending interrupts */
-		err = at803x_ack_interrupt(phydev);
-		if (err)
-			return err;
-
-		value |= AT803X_INTR_ENABLE_AUTONEG_ERR;
-		value |= AT803X_INTR_ENABLE_SPEED_CHANGED;
-		value |= AT803X_INTR_ENABLE_DUPLEX_CHANGED;
-		value |= AT803X_INTR_ENABLE_LINK_FAIL;
-		value |= AT803X_INTR_ENABLE_LINK_SUCCESS;
-
-		err = phy_write(phydev, AT803X_INTR_ENABLE, value);
-	} else {
-		err = phy_write(phydev, AT803X_INTR_ENABLE, 0);
-		if (err)
-			return err;
-
-		/* Clear any pending interrupts */
-		err = at803x_ack_interrupt(phydev);
-	}
-
-	return err;
-}
-
-static irqreturn_t at803x_handle_interrupt(struct phy_device *phydev)
-{
-	int irq_status, int_enabled;
-
-	irq_status = phy_read(phydev, AT803X_INTR_STATUS);
-	if (irq_status < 0) {
-		phy_error(phydev);
-		return IRQ_NONE;
-	}
-
-	/* Read the current enabled interrupts */
-	int_enabled = phy_read(phydev, AT803X_INTR_ENABLE);
-	if (int_enabled < 0) {
-		phy_error(phydev);
-		return IRQ_NONE;
-	}
-
-	/* See if this was one of our enabled interrupts */
-	if (!(irq_status & int_enabled))
-		return IRQ_NONE;
-
-	phy_trigger_machine(phydev);
-
-	return IRQ_HANDLED;
-}
-
 static void at803x_link_change_notify(struct phy_device *phydev)
 {
 	/*
@@ -767,69 +474,6 @@ static void at803x_link_change_notify(struct phy_device *phydev)
 	}
 }
 
-static int at803x_read_specific_status(struct phy_device *phydev,
-				       struct at803x_ss_mask ss_mask)
-{
-	int ss;
-
-	/* Read the AT8035 PHY-Specific Status register, which indicates the
-	 * speed and duplex that the PHY is actually using, irrespective of
-	 * whether we are in autoneg mode or not.
-	 */
-	ss = phy_read(phydev, AT803X_SPECIFIC_STATUS);
-	if (ss < 0)
-		return ss;
-
-	if (ss & AT803X_SS_SPEED_DUPLEX_RESOLVED) {
-		int sfc, speed;
-
-		sfc = phy_read(phydev, AT803X_SPECIFIC_FUNCTION_CONTROL);
-		if (sfc < 0)
-			return sfc;
-
-		speed = ss & ss_mask.speed_mask;
-		speed >>= ss_mask.speed_shift;
-
-		switch (speed) {
-		case AT803X_SS_SPEED_10:
-			phydev->speed = SPEED_10;
-			break;
-		case AT803X_SS_SPEED_100:
-			phydev->speed = SPEED_100;
-			break;
-		case AT803X_SS_SPEED_1000:
-			phydev->speed = SPEED_1000;
-			break;
-		case QCA808X_SS_SPEED_2500:
-			phydev->speed = SPEED_2500;
-			break;
-		}
-		if (ss & AT803X_SS_DUPLEX)
-			phydev->duplex = DUPLEX_FULL;
-		else
-			phydev->duplex = DUPLEX_HALF;
-
-		if (ss & AT803X_SS_MDIX)
-			phydev->mdix = ETH_TP_MDI_X;
-		else
-			phydev->mdix = ETH_TP_MDI;
-
-		switch (FIELD_GET(AT803X_SFC_MDI_CROSSOVER_MODE_M, sfc)) {
-		case AT803X_SFC_MANUAL_MDI:
-			phydev->mdix_ctrl = ETH_TP_MDI;
-			break;
-		case AT803X_SFC_MANUAL_MDIX:
-			phydev->mdix_ctrl = ETH_TP_MDI_X;
-			break;
-		case AT803X_SFC_AUTOMATIC_CROSSOVER:
-			phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
-			break;
-		}
-	}
-
-	return 0;
-}
-
 static int at803x_read_status(struct phy_device *phydev)
 {
 	struct at803x_ss_mask ss_mask = { 0 };
@@ -865,29 +509,6 @@ static int at803x_read_status(struct phy_device *phydev)
 	return 0;
 }
 
-static int at803x_config_mdix(struct phy_device *phydev, u8 ctrl)
-{
-	u16 val;
-
-	switch (ctrl) {
-	case ETH_TP_MDI:
-		val = AT803X_SFC_MANUAL_MDI;
-		break;
-	case ETH_TP_MDI_X:
-		val = AT803X_SFC_MANUAL_MDIX;
-		break;
-	case ETH_TP_MDI_AUTO:
-		val = AT803X_SFC_AUTOMATIC_CROSSOVER;
-		break;
-	default:
-		return 0;
-	}
-
-	return phy_modify_changed(phydev, AT803X_SPECIFIC_FUNCTION_CONTROL,
-			  AT803X_SFC_MDI_CROSSOVER_MODE_M,
-			  FIELD_PREP(AT803X_SFC_MDI_CROSSOVER_MODE_M, val));
-}
-
 static int at803x_config_aneg(struct phy_device *phydev)
 {
 	int ret;
@@ -914,80 +535,6 @@ static int at803x_config_aneg(struct phy_device *phydev)
 	return __genphy_config_aneg(phydev, ret);
 }
 
-static int at803x_get_downshift(struct phy_device *phydev, u8 *d)
-{
-	int val;
-
-	val = phy_read(phydev, AT803X_SMART_SPEED);
-	if (val < 0)
-		return val;
-
-	if (val & AT803X_SMART_SPEED_ENABLE)
-		*d = FIELD_GET(AT803X_SMART_SPEED_RETRY_LIMIT_MASK, val) + 2;
-	else
-		*d = DOWNSHIFT_DEV_DISABLE;
-
-	return 0;
-}
-
-static int at803x_set_downshift(struct phy_device *phydev, u8 cnt)
-{
-	u16 mask, set;
-	int ret;
-
-	switch (cnt) {
-	case DOWNSHIFT_DEV_DEFAULT_COUNT:
-		cnt = AT803X_DEFAULT_DOWNSHIFT;
-		fallthrough;
-	case AT803X_MIN_DOWNSHIFT ... AT803X_MAX_DOWNSHIFT:
-		set = AT803X_SMART_SPEED_ENABLE |
-		      AT803X_SMART_SPEED_BYPASS_TIMER |
-		      FIELD_PREP(AT803X_SMART_SPEED_RETRY_LIMIT_MASK, cnt - 2);
-		mask = AT803X_SMART_SPEED_RETRY_LIMIT_MASK;
-		break;
-	case DOWNSHIFT_DEV_DISABLE:
-		set = 0;
-		mask = AT803X_SMART_SPEED_ENABLE |
-		       AT803X_SMART_SPEED_BYPASS_TIMER;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	ret = phy_modify_changed(phydev, AT803X_SMART_SPEED, mask, set);
-
-	/* After changing the smart speed settings, we need to perform a
-	 * software reset, use phy_init_hw() to make sure we set the
-	 * reapply any values which might got lost during software reset.
-	 */
-	if (ret == 1)
-		ret = phy_init_hw(phydev);
-
-	return ret;
-}
-
-static int at803x_get_tunable(struct phy_device *phydev,
-			      struct ethtool_tunable *tuna, void *data)
-{
-	switch (tuna->id) {
-	case ETHTOOL_PHY_DOWNSHIFT:
-		return at803x_get_downshift(phydev, data);
-	default:
-		return -EOPNOTSUPP;
-	}
-}
-
-static int at803x_set_tunable(struct phy_device *phydev,
-			      struct ethtool_tunable *tuna, const void *data)
-{
-	switch (tuna->id) {
-	case ETHTOOL_PHY_DOWNSHIFT:
-		return at803x_set_downshift(phydev, *(const u8 *)data);
-	default:
-		return -EOPNOTSUPP;
-	}
-}
-
 static int at803x_cable_test_result_trans(u16 status)
 {
 	switch (FIELD_GET(AT803X_CDT_STATUS_STAT_MASK, status)) {
@@ -1551,423 +1098,6 @@ static int at8035_probe(struct phy_device *phydev)
 	return at8035_parse_dt(phydev);
 }
 
-static int qca808x_config_aneg(struct phy_device *phydev)
-{
-	int phy_ctrl = 0;
-	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;
-	}
-
-	/* Do not restart auto-negotiation by setting ret to 0 defautly,
-	 * when calling __genphy_config_aneg later.
-	 */
-	ret = 0;
-
-	/* The reg MII_BMCR also needs to be configured for force mode, the
-	 * genphy_config_aneg is also needed.
-	 */
-	if (phydev->autoneg == AUTONEG_DISABLE)
-		genphy_c45_pma_setup_forced(phydev);
-
-	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->advertising))
-		phy_ctrl = MDIO_AN_10GBT_CTRL_ADV2_5G;
-
-	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
-				     MDIO_AN_10GBT_CTRL_ADV2_5G, phy_ctrl);
-	if (ret < 0)
-		return ret;
-
-	return __genphy_config_aneg(phydev, ret);
-}
-
-static int qca808x_phy_fast_retrain_config(struct phy_device *phydev)
-{
-	int ret;
-
-	/* Enable fast retrain */
-	ret = genphy_c45_fast_retrain(phydev, true);
-	if (ret)
-		return ret;
-
-	phy_write_mmd(phydev, MDIO_MMD_AN, QCA808X_PHY_MMD7_TOP_OPTION1,
-			QCA808X_TOP_OPTION1_DATA);
-	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, QCA808X_PHY_MMD1_MSE_THRESHOLD_20DB,
-			QCA808X_MSE_THRESHOLD_20DB_VALUE);
-	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, QCA808X_PHY_MMD1_MSE_THRESHOLD_17DB,
-			QCA808X_MSE_THRESHOLD_17DB_VALUE);
-	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, QCA808X_PHY_MMD1_MSE_THRESHOLD_27DB,
-			QCA808X_MSE_THRESHOLD_27DB_VALUE);
-	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, QCA808X_PHY_MMD1_MSE_THRESHOLD_28DB,
-			QCA808X_MSE_THRESHOLD_28DB_VALUE);
-	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_1,
-			QCA808X_MMD3_DEBUG_1_VALUE);
-	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_4,
-			QCA808X_MMD3_DEBUG_4_VALUE);
-	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_5,
-			QCA808X_MMD3_DEBUG_5_VALUE);
-	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_3,
-			QCA808X_MMD3_DEBUG_3_VALUE);
-	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_6,
-			QCA808X_MMD3_DEBUG_6_VALUE);
-	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_2,
-			QCA808X_MMD3_DEBUG_2_VALUE);
-
-	return 0;
-}
-
-static int qca808x_phy_ms_seed_enable(struct phy_device *phydev, bool enable)
-{
-	u16 seed_value;
-
-	if (!enable)
-		return at803x_debug_reg_mask(phydev, QCA808X_PHY_DEBUG_LOCAL_SEED,
-				QCA808X_MASTER_SLAVE_SEED_ENABLE, 0);
-
-	seed_value = get_random_u32_below(QCA808X_MASTER_SLAVE_SEED_RANGE);
-	return at803x_debug_reg_mask(phydev, QCA808X_PHY_DEBUG_LOCAL_SEED,
-			QCA808X_MASTER_SLAVE_SEED_CFG | QCA808X_MASTER_SLAVE_SEED_ENABLE,
-			FIELD_PREP(QCA808X_MASTER_SLAVE_SEED_CFG, seed_value) |
-			QCA808X_MASTER_SLAVE_SEED_ENABLE);
-}
-
-static bool qca808x_is_prefer_master(struct phy_device *phydev)
-{
-	return (phydev->master_slave_get == MASTER_SLAVE_CFG_MASTER_FORCE) ||
-		(phydev->master_slave_get == MASTER_SLAVE_CFG_MASTER_PREFERRED);
-}
-
-static bool qca808x_has_fast_retrain_or_slave_seed(struct phy_device *phydev)
-{
-	return linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->supported);
-}
-
-static int qca808x_config_init(struct phy_device *phydev)
-{
-	int ret;
-
-	/* Active adc&vga on 802.3az for the link 1000M and 100M */
-	ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_ADDR_CLD_CTRL7,
-			QCA808X_8023AZ_AFE_CTRL_MASK, QCA808X_8023AZ_AFE_EN);
-	if (ret)
-		return ret;
-
-	/* Adjust the threshold on 802.3az for the link 1000M */
-	ret = phy_write_mmd(phydev, MDIO_MMD_PCS,
-			QCA808X_PHY_MMD3_AZ_TRAINING_CTRL, QCA808X_MMD3_AZ_TRAINING_VAL);
-	if (ret)
-		return ret;
-
-	if (qca808x_has_fast_retrain_or_slave_seed(phydev)) {
-		/* Config the fast retrain for the link 2500M */
-		ret = qca808x_phy_fast_retrain_config(phydev);
-		if (ret)
-			return ret;
-
-		ret = genphy_read_master_slave(phydev);
-		if (ret < 0)
-			return ret;
-
-		if (!qca808x_is_prefer_master(phydev)) {
-			/* Enable seed and configure lower ramdom seed to make phy
-			 * linked as slave mode.
-			 */
-			ret = qca808x_phy_ms_seed_enable(phydev, true);
-			if (ret)
-				return ret;
-		}
-	}
-
-	/* Configure adc threshold as 100mv for the link 10M */
-	return at803x_debug_reg_mask(phydev, QCA808X_PHY_DEBUG_ADC_THRESHOLD,
-			QCA808X_ADC_THRESHOLD_MASK, QCA808X_ADC_THRESHOLD_100MV);
-}
-
-static int qca808x_read_status(struct phy_device *phydev)
-{
-	struct at803x_ss_mask ss_mask = { 0 };
-	int ret;
-
-	ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_STAT);
-	if (ret < 0)
-		return ret;
-
-	linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->lp_advertising,
-			ret & MDIO_AN_10GBT_STAT_LP2_5G);
-
-	ret = genphy_read_status(phydev);
-	if (ret)
-		return ret;
-
-	/* qca8081 takes the different bits for speed value from at803x */
-	ss_mask.speed_mask = QCA808X_SS_SPEED_MASK;
-	ss_mask.speed_shift = __bf_shf(QCA808X_SS_SPEED_MASK);
-	ret = at803x_read_specific_status(phydev, ss_mask);
-	if (ret < 0)
-		return ret;
-
-	if (phydev->link) {
-		if (phydev->speed == SPEED_2500)
-			phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
-		else
-			phydev->interface = PHY_INTERFACE_MODE_SGMII;
-	} else {
-		/* generate seed as a lower random value to make PHY linked as SLAVE easily,
-		 * except for master/slave configuration fault detected or the master mode
-		 * preferred.
-		 *
-		 * the reason for not putting this code into the function link_change_notify is
-		 * the corner case where the link partner is also the qca8081 PHY and the seed
-		 * value is configured as the same value, the link can't be up and no link change
-		 * occurs.
-		 */
-		if (qca808x_has_fast_retrain_or_slave_seed(phydev)) {
-			if (phydev->master_slave_state == MASTER_SLAVE_STATE_ERR ||
-					qca808x_is_prefer_master(phydev)) {
-				qca808x_phy_ms_seed_enable(phydev, false);
-			} else {
-				qca808x_phy_ms_seed_enable(phydev, true);
-			}
-		}
-	}
-
-	return 0;
-}
-
-static int qca808x_soft_reset(struct phy_device *phydev)
-{
-	int ret;
-
-	ret = genphy_soft_reset(phydev);
-	if (ret < 0)
-		return ret;
-
-	if (qca808x_has_fast_retrain_or_slave_seed(phydev))
-		ret = qca808x_phy_ms_seed_enable(phydev, true);
-
-	return ret;
-}
-
-static bool qca808x_cdt_fault_length_valid(int cdt_code)
-{
-	switch (cdt_code) {
-	case QCA808X_CDT_STATUS_STAT_SHORT:
-	case QCA808X_CDT_STATUS_STAT_OPEN:
-		return true;
-	default:
-		return false;
-	}
-}
-
-static int qca808x_cable_test_result_trans(int cdt_code)
-{
-	switch (cdt_code) {
-	case QCA808X_CDT_STATUS_STAT_NORMAL:
-		return ETHTOOL_A_CABLE_RESULT_CODE_OK;
-	case QCA808X_CDT_STATUS_STAT_SHORT:
-		return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
-	case QCA808X_CDT_STATUS_STAT_OPEN:
-		return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
-	case QCA808X_CDT_STATUS_STAT_FAIL:
-	default:
-		return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
-	}
-}
-
-static int qca808x_cdt_fault_length(struct phy_device *phydev, int pair)
-{
-	int val;
-	u32 cdt_length_reg = 0;
-
-	switch (pair) {
-	case ETHTOOL_A_CABLE_PAIR_A:
-		cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_A;
-		break;
-	case ETHTOOL_A_CABLE_PAIR_B:
-		cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_B;
-		break;
-	case ETHTOOL_A_CABLE_PAIR_C:
-		cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_C;
-		break;
-	case ETHTOOL_A_CABLE_PAIR_D:
-		cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_D;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	val = phy_read_mmd(phydev, MDIO_MMD_PCS, cdt_length_reg);
-	if (val < 0)
-		return val;
-
-	return (FIELD_GET(QCA808X_CDT_DIAG_LENGTH, val) * 824) / 10;
-}
-
-static int qca808x_cable_test_start(struct phy_device *phydev)
-{
-	int ret;
-
-	/* perform CDT with the following configs:
-	 * 1. disable hibernation.
-	 * 2. force PHY working in MDI mode.
-	 * 3. for PHY working in 1000BaseT.
-	 * 4. configure the threshold.
-	 */
-
-	ret = at803x_debug_reg_mask(phydev, QCA808X_DBG_AN_TEST, QCA808X_HIBERNATION_EN, 0);
-	if (ret < 0)
-		return ret;
-
-	ret = at803x_config_mdix(phydev, ETH_TP_MDI);
-	if (ret < 0)
-		return ret;
-
-	/* Force 1000base-T needs to configure PMA/PMD and MII_BMCR */
-	phydev->duplex = DUPLEX_FULL;
-	phydev->speed = SPEED_1000;
-	ret = genphy_c45_pma_setup_forced(phydev);
-	if (ret < 0)
-		return ret;
-
-	ret = genphy_setup_forced(phydev);
-	if (ret < 0)
-		return ret;
-
-	/* configure the thresholds for open, short, pair ok test */
-	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x8074, 0xc040);
-	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x8076, 0xc040);
-	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x8077, 0xa060);
-	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x8078, 0xc050);
-	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x807a, 0xc060);
-	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x807e, 0xb060);
-
-	return 0;
-}
-
-static int qca808x_cdt_start(struct phy_device *phydev)
-{
-	u16 cdt;
-
-	/* qca8081 takes the different bit 15 to enable CDT test */
-	cdt = QCA808X_CDT_ENABLE_TEST |
-	      QCA808X_CDT_LENGTH_UNIT |
-	      QCA808X_CDT_INTER_CHECK_DIS;
-
-	return phy_write(phydev, AT803X_CDT, cdt);
-}
-
-static int qca808x_cdt_wait_for_completition(struct phy_device *phydev)
-{
-	int val, ret;
-
-	/* One test run takes about 25ms */
-	ret = phy_read_poll_timeout(phydev, AT803X_CDT, val,
-				    !(val & QCA808X_CDT_ENABLE_TEST),
-				    30000, 100000, true);
-
-	return ret < 0 ? ret : 0;
-}
-
-static int qca808x_cable_test_get_status(struct phy_device *phydev, bool *finished)
-{
-	int ret, val;
-	int pair_a, pair_b, pair_c, pair_d;
-
-	*finished = false;
-
-	ret = qca808x_cdt_start(phydev);
-	if (ret)
-		return ret;
-
-	ret = qca808x_cdt_wait_for_completition(phydev);
-	if (ret)
-		return ret;
-
-	val = phy_read_mmd(phydev, MDIO_MMD_PCS, QCA808X_MMD3_CDT_STATUS);
-	if (val < 0)
-		return val;
-
-	pair_a = FIELD_GET(QCA808X_CDT_CODE_PAIR_A, val);
-	pair_b = FIELD_GET(QCA808X_CDT_CODE_PAIR_B, val);
-	pair_c = FIELD_GET(QCA808X_CDT_CODE_PAIR_C, val);
-	pair_d = FIELD_GET(QCA808X_CDT_CODE_PAIR_D, val);
-
-	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
-				qca808x_cable_test_result_trans(pair_a));
-	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_B,
-				qca808x_cable_test_result_trans(pair_b));
-	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_C,
-				qca808x_cable_test_result_trans(pair_c));
-	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_D,
-				qca808x_cable_test_result_trans(pair_d));
-
-	if (qca808x_cdt_fault_length_valid(pair_a))
-		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A,
-				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A));
-	if (qca808x_cdt_fault_length_valid(pair_b))
-		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B,
-				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B));
-	if (qca808x_cdt_fault_length_valid(pair_c))
-		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C,
-				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C));
-	if (qca808x_cdt_fault_length_valid(pair_d))
-		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D,
-				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D));
-
-	*finished = true;
-
-	return 0;
-}
-
-static int qca808x_get_features(struct phy_device *phydev)
-{
-	int ret;
-
-	ret = genphy_c45_pma_read_abilities(phydev);
-	if (ret)
-		return ret;
-
-	/* The autoneg ability is not existed in bit3 of MMD7.1,
-	 * but it is supported by qca808x PHY, so we add it here
-	 * manually.
-	 */
-	linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported);
-
-	/* As for the qca8081 1G version chip, the 2500baseT ability is also
-	 * existed in the bit0 of MMD1.21, we need to remove it manually if
-	 * it is the qca8081 1G chip according to the bit0 of MMD7.0x901d.
-	 */
-	ret = phy_read_mmd(phydev, MDIO_MMD_AN, QCA808X_PHY_MMD7_CHIP_TYPE);
-	if (ret < 0)
-		return ret;
-
-	if (QCA808X_PHY_CHIP_TYPE_1G & ret)
-		linkmode_clear_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->supported);
-
-	return 0;
-}
-
-static void qca808x_link_change_notify(struct phy_device *phydev)
-{
-	/* Assert interface sgmii fifo on link down, deassert it on link up,
-	 * the interface device address is always phy address added by 1.
-	 */
-	mdiobus_c45_modify_changed(phydev->mdio.bus, phydev->mdio.addr + 1,
-			MDIO_MMD_PMAPMD, QCA8081_PHY_SERDES_MMD1_FIFO_CTRL,
-			QCA8081_PHY_FIFO_RSTN, phydev->link ? QCA8081_PHY_FIFO_RSTN : 0);
-}
-
 static struct phy_driver at803x_driver[] = {
 {
 	/* Qualcomm Atheros AR8035 */
@@ -2075,27 +1205,6 @@ static struct phy_driver at803x_driver[] = {
 	.read_status		= at803x_read_status,
 	.soft_reset		= genphy_soft_reset,
 	.config_aneg		= at803x_config_aneg,
-}, {
-	/* Qualcomm QCA8081 */
-	PHY_ID_MATCH_EXACT(QCA8081_PHY_ID),
-	.name			= "Qualcomm QCA8081",
-	.flags			= PHY_POLL_CABLE_TEST,
-	.config_intr		= at803x_config_intr,
-	.handle_interrupt	= at803x_handle_interrupt,
-	.get_tunable		= at803x_get_tunable,
-	.set_tunable		= at803x_set_tunable,
-	.set_wol		= at803x_set_wol,
-	.get_wol		= at803x_get_wol,
-	.get_features		= qca808x_get_features,
-	.config_aneg		= qca808x_config_aneg,
-	.suspend		= genphy_suspend,
-	.resume			= genphy_resume,
-	.read_status		= qca808x_read_status,
-	.config_init		= qca808x_config_init,
-	.soft_reset		= qca808x_soft_reset,
-	.cable_test_start	= qca808x_cable_test_start,
-	.cable_test_get_status	= qca808x_cable_test_get_status,
-	.link_change_notify	= qca808x_link_change_notify,
 }, };
 
 module_phy_driver(at803x_driver);
@@ -2107,7 +1216,6 @@ static struct mdio_device_id __maybe_unused atheros_tbl[] = {
 	{ PHY_ID_MATCH_EXACT(ATH8035_PHY_ID) },
 	{ PHY_ID_MATCH_EXACT(ATH9331_PHY_ID) },
 	{ PHY_ID_MATCH_EXACT(QCA9561_PHY_ID) },
-	{ PHY_ID_MATCH_EXACT(QCA8081_PHY_ID) },
 	{ }
 };
 
diff --git a/drivers/net/phy/qcom/common.c b/drivers/net/phy/qcom/common.c
index 1d9b80fea2e9..48c1614eb979 100644
--- a/drivers/net/phy/qcom/common.c
+++ b/drivers/net/phy/qcom/common.c
@@ -2,7 +2,7 @@
 
 #include <linux/phy.h>
 #include <linux/module.h>
-
+#include <linux/etherdevice.h>
 #include <linux/regulator/driver.h>
 #include <linux/of.h>
 #include <linux/phylink.h>
@@ -48,3 +48,304 @@ int at803x_debug_reg_write(struct phy_device *phydev, u16 reg, u16 data)
 
 	return phy_write(phydev, AT803X_DEBUG_DATA, data);
 }
+
+int at803x_config_mdix(struct phy_device *phydev, u8 ctrl)
+{
+	u16 val;
+
+	switch (ctrl) {
+	case ETH_TP_MDI:
+		val = AT803X_SFC_MANUAL_MDI;
+		break;
+	case ETH_TP_MDI_X:
+		val = AT803X_SFC_MANUAL_MDIX;
+		break;
+	case ETH_TP_MDI_AUTO:
+		val = AT803X_SFC_AUTOMATIC_CROSSOVER;
+		break;
+	default:
+		return 0;
+	}
+
+	return phy_modify_changed(phydev, AT803X_SPECIFIC_FUNCTION_CONTROL,
+			  AT803X_SFC_MDI_CROSSOVER_MODE_M,
+			  FIELD_PREP(AT803X_SFC_MDI_CROSSOVER_MODE_M, val));
+}
+
+int at803x_read_specific_status(struct phy_device *phydev,
+				struct at803x_ss_mask ss_mask)
+{
+	int ss;
+
+	/* Read the AT8035 PHY-Specific Status register, which indicates the
+	 * speed and duplex that the PHY is actually using, irrespective of
+	 * whether we are in autoneg mode or not.
+	 */
+	ss = phy_read(phydev, AT803X_SPECIFIC_STATUS);
+	if (ss < 0)
+		return ss;
+
+	if (ss & AT803X_SS_SPEED_DUPLEX_RESOLVED) {
+		int sfc, speed;
+
+		sfc = phy_read(phydev, AT803X_SPECIFIC_FUNCTION_CONTROL);
+		if (sfc < 0)
+			return sfc;
+
+		speed = ss & ss_mask.speed_mask;
+		speed >>= ss_mask.speed_shift;
+
+		switch (speed) {
+		case AT803X_SS_SPEED_10:
+			phydev->speed = SPEED_10;
+			break;
+		case AT803X_SS_SPEED_100:
+			phydev->speed = SPEED_100;
+			break;
+		case AT803X_SS_SPEED_1000:
+			phydev->speed = SPEED_1000;
+			break;
+		case QCA808X_SS_SPEED_2500:
+			phydev->speed = SPEED_2500;
+			break;
+		}
+		if (ss & AT803X_SS_DUPLEX)
+			phydev->duplex = DUPLEX_FULL;
+		else
+			phydev->duplex = DUPLEX_HALF;
+
+		if (ss & AT803X_SS_MDIX)
+			phydev->mdix = ETH_TP_MDI_X;
+		else
+			phydev->mdix = ETH_TP_MDI;
+
+		switch (FIELD_GET(AT803X_SFC_MDI_CROSSOVER_MODE_M, sfc)) {
+		case AT803X_SFC_MANUAL_MDI:
+			phydev->mdix_ctrl = ETH_TP_MDI;
+			break;
+		case AT803X_SFC_MANUAL_MDIX:
+			phydev->mdix_ctrl = ETH_TP_MDI_X;
+			break;
+		case AT803X_SFC_AUTOMATIC_CROSSOVER:
+			phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
+			break;
+		}
+	}
+
+	return 0;
+}
+
+int at803x_config_intr(struct phy_device *phydev)
+{
+	int err;
+	int value;
+
+	value = phy_read(phydev, AT803X_INTR_ENABLE);
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		/* Clear any pending interrupts */
+		err = at803x_ack_interrupt(phydev);
+		if (err)
+			return err;
+
+		value |= AT803X_INTR_ENABLE_AUTONEG_ERR;
+		value |= AT803X_INTR_ENABLE_SPEED_CHANGED;
+		value |= AT803X_INTR_ENABLE_DUPLEX_CHANGED;
+		value |= AT803X_INTR_ENABLE_LINK_FAIL;
+		value |= AT803X_INTR_ENABLE_LINK_SUCCESS;
+
+		err = phy_write(phydev, AT803X_INTR_ENABLE, value);
+	} else {
+		err = phy_write(phydev, AT803X_INTR_ENABLE, 0);
+		if (err)
+			return err;
+
+		/* Clear any pending interrupts */
+		err = at803x_ack_interrupt(phydev);
+	}
+
+	return err;
+}
+
+int at803x_ack_interrupt(struct phy_device *phydev)
+{
+	int err;
+
+	err = phy_read(phydev, AT803X_INTR_STATUS);
+
+	return (err < 0) ? err : 0;
+}
+
+irqreturn_t at803x_handle_interrupt(struct phy_device *phydev)
+{
+	int irq_status, int_enabled;
+
+	irq_status = phy_read(phydev, AT803X_INTR_STATUS);
+	if (irq_status < 0) {
+		phy_error(phydev);
+		return IRQ_NONE;
+	}
+
+	/* Read the current enabled interrupts */
+	int_enabled = phy_read(phydev, AT803X_INTR_ENABLE);
+	if (int_enabled < 0) {
+		phy_error(phydev);
+		return IRQ_NONE;
+	}
+
+	/* See if this was one of our enabled interrupts */
+	if (!(irq_status & int_enabled))
+		return IRQ_NONE;
+
+	phy_trigger_machine(phydev);
+
+	return IRQ_HANDLED;
+}
+
+int at803x_get_tunable(struct phy_device *phydev,
+		       struct ethtool_tunable *tuna, void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_DOWNSHIFT:
+		return at803x_get_downshift(phydev, data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+int at803x_set_tunable(struct phy_device *phydev,
+		       struct ethtool_tunable *tuna, const void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_DOWNSHIFT:
+		return at803x_set_downshift(phydev, *(const u8 *)data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+int at803x_get_downshift(struct phy_device *phydev, u8 *d)
+{
+	int val;
+
+	val = phy_read(phydev, AT803X_SMART_SPEED);
+	if (val < 0)
+		return val;
+
+	if (val & AT803X_SMART_SPEED_ENABLE)
+		*d = FIELD_GET(AT803X_SMART_SPEED_RETRY_LIMIT_MASK, val) + 2;
+	else
+		*d = DOWNSHIFT_DEV_DISABLE;
+
+	return 0;
+}
+
+int at803x_set_downshift(struct phy_device *phydev, u8 cnt)
+{
+	u16 mask, set;
+	int ret;
+
+	switch (cnt) {
+	case DOWNSHIFT_DEV_DEFAULT_COUNT:
+		cnt = AT803X_DEFAULT_DOWNSHIFT;
+		fallthrough;
+	case AT803X_MIN_DOWNSHIFT ... AT803X_MAX_DOWNSHIFT:
+		set = AT803X_SMART_SPEED_ENABLE |
+		      AT803X_SMART_SPEED_BYPASS_TIMER |
+		      FIELD_PREP(AT803X_SMART_SPEED_RETRY_LIMIT_MASK, cnt - 2);
+		mask = AT803X_SMART_SPEED_RETRY_LIMIT_MASK;
+		break;
+	case DOWNSHIFT_DEV_DISABLE:
+		set = 0;
+		mask = AT803X_SMART_SPEED_ENABLE |
+		       AT803X_SMART_SPEED_BYPASS_TIMER;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = phy_modify_changed(phydev, AT803X_SMART_SPEED, mask, set);
+
+	/* After changing the smart speed settings, we need to perform a
+	 * software reset, use phy_init_hw() to make sure we set the
+	 * reapply any values which might got lost during software reset.
+	 */
+	if (ret == 1)
+		ret = phy_init_hw(phydev);
+
+	return ret;
+}
+
+int at803x_set_wol(struct phy_device *phydev,
+		   struct ethtool_wolinfo *wol)
+{
+	int ret, irq_enabled;
+
+	if (wol->wolopts & WAKE_MAGIC) {
+		struct net_device *ndev = phydev->attached_dev;
+		const u8 *mac;
+		unsigned int i;
+		static const unsigned int offsets[] = {
+			AT803X_LOC_MAC_ADDR_32_47_OFFSET,
+			AT803X_LOC_MAC_ADDR_16_31_OFFSET,
+			AT803X_LOC_MAC_ADDR_0_15_OFFSET,
+		};
+
+		if (!ndev)
+			return -ENODEV;
+
+		mac = (const u8 *)ndev->dev_addr;
+
+		if (!is_valid_ether_addr(mac))
+			return -EINVAL;
+
+		for (i = 0; i < 3; i++)
+			phy_write_mmd(phydev, MDIO_MMD_PCS, offsets[i],
+				      mac[(i * 2) + 1] | (mac[(i * 2)] << 8));
+
+		/* Enable WOL interrupt */
+		ret = phy_modify(phydev, AT803X_INTR_ENABLE, 0, AT803X_INTR_ENABLE_WOL);
+		if (ret)
+			return ret;
+	} else {
+		/* Disable WOL interrupt */
+		ret = phy_modify(phydev, AT803X_INTR_ENABLE, AT803X_INTR_ENABLE_WOL, 0);
+		if (ret)
+			return ret;
+	}
+
+	/* Clear WOL status */
+	ret = phy_read(phydev, AT803X_INTR_STATUS);
+	if (ret < 0)
+		return ret;
+
+	/* Check if there are other interrupts except for WOL triggered when PHY is
+	 * in interrupt mode, only the interrupts enabled by AT803X_INTR_ENABLE can
+	 * be passed up to the interrupt PIN.
+	 */
+	irq_enabled = phy_read(phydev, AT803X_INTR_ENABLE);
+	if (irq_enabled < 0)
+		return irq_enabled;
+
+	irq_enabled &= ~AT803X_INTR_ENABLE_WOL;
+	if (ret & irq_enabled && !phy_polling_mode(phydev))
+		phy_trigger_machine(phydev);
+
+	return 0;
+}
+
+void at803x_get_wol(struct phy_device *phydev,
+		    struct ethtool_wolinfo *wol)
+{
+	int value;
+
+	wol->supported = WAKE_MAGIC;
+	wol->wolopts = 0;
+
+	value = phy_read(phydev, AT803X_INTR_ENABLE);
+	if (value < 0)
+		return;
+
+	if (value & AT803X_INTR_ENABLE_WOL)
+		wol->wolopts |= WAKE_MAGIC;
+}
diff --git a/drivers/net/phy/qcom/qca808x.c b/drivers/net/phy/qcom/qca808x.c
new file mode 100644
index 000000000000..61bc98bc8869
--- /dev/null
+++ b/drivers/net/phy/qcom/qca808x.c
@@ -0,0 +1,550 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/phy.h>
+#include <linux/module.h>
+#include <linux/ethtool_netlink.h>
+
+#include "qcom.h"
+
+/* ADC threshold */
+#define QCA808X_PHY_DEBUG_ADC_THRESHOLD		0x2c80
+#define QCA808X_ADC_THRESHOLD_MASK		GENMASK(7, 0)
+#define QCA808X_ADC_THRESHOLD_80MV		0
+#define QCA808X_ADC_THRESHOLD_100MV		0xf0
+#define QCA808X_ADC_THRESHOLD_200MV		0x0f
+#define QCA808X_ADC_THRESHOLD_300MV		0xff
+
+/* CLD control */
+#define QCA808X_PHY_MMD3_ADDR_CLD_CTRL7		0x8007
+#define QCA808X_8023AZ_AFE_CTRL_MASK		GENMASK(8, 4)
+#define QCA808X_8023AZ_AFE_EN			0x90
+
+/* AZ control */
+#define QCA808X_PHY_MMD3_AZ_TRAINING_CTRL	0x8008
+#define QCA808X_MMD3_AZ_TRAINING_VAL		0x1c32
+
+#define QCA808X_PHY_MMD1_MSE_THRESHOLD_20DB	0x8014
+#define QCA808X_MSE_THRESHOLD_20DB_VALUE	0x529
+
+#define QCA808X_PHY_MMD1_MSE_THRESHOLD_17DB	0x800E
+#define QCA808X_MSE_THRESHOLD_17DB_VALUE	0x341
+
+#define QCA808X_PHY_MMD1_MSE_THRESHOLD_27DB	0x801E
+#define QCA808X_MSE_THRESHOLD_27DB_VALUE	0x419
+
+#define QCA808X_PHY_MMD1_MSE_THRESHOLD_28DB	0x8020
+#define QCA808X_MSE_THRESHOLD_28DB_VALUE	0x341
+
+#define QCA808X_PHY_MMD7_TOP_OPTION1		0x901c
+#define QCA808X_TOP_OPTION1_DATA		0x0
+
+#define QCA808X_PHY_MMD3_DEBUG_1		0xa100
+#define QCA808X_MMD3_DEBUG_1_VALUE		0x9203
+#define QCA808X_PHY_MMD3_DEBUG_2		0xa101
+#define QCA808X_MMD3_DEBUG_2_VALUE		0x48ad
+#define QCA808X_PHY_MMD3_DEBUG_3		0xa103
+#define QCA808X_MMD3_DEBUG_3_VALUE		0x1698
+#define QCA808X_PHY_MMD3_DEBUG_4		0xa105
+#define QCA808X_MMD3_DEBUG_4_VALUE		0x8001
+#define QCA808X_PHY_MMD3_DEBUG_5		0xa106
+#define QCA808X_MMD3_DEBUG_5_VALUE		0x1111
+#define QCA808X_PHY_MMD3_DEBUG_6		0xa011
+#define QCA808X_MMD3_DEBUG_6_VALUE		0x5f85
+
+/* master/slave seed config */
+#define QCA808X_PHY_DEBUG_LOCAL_SEED		9
+#define QCA808X_MASTER_SLAVE_SEED_ENABLE	BIT(1)
+#define QCA808X_MASTER_SLAVE_SEED_CFG		GENMASK(12, 2)
+#define QCA808X_MASTER_SLAVE_SEED_RANGE		0x32
+
+/* Hibernation yields lower power consumpiton in contrast with normal operation mode.
+ * when the copper cable is unplugged, the PHY enters into hibernation mode in about 10s.
+ */
+#define QCA808X_DBG_AN_TEST			0xb
+#define QCA808X_HIBERNATION_EN			BIT(15)
+
+#define QCA808X_CDT_ENABLE_TEST			BIT(15)
+#define QCA808X_CDT_INTER_CHECK_DIS		BIT(13)
+#define QCA808X_CDT_LENGTH_UNIT			BIT(10)
+
+#define QCA808X_MMD3_CDT_STATUS			0x8064
+#define QCA808X_MMD3_CDT_DIAG_PAIR_A		0x8065
+#define QCA808X_MMD3_CDT_DIAG_PAIR_B		0x8066
+#define QCA808X_MMD3_CDT_DIAG_PAIR_C		0x8067
+#define QCA808X_MMD3_CDT_DIAG_PAIR_D		0x8068
+#define QCA808X_CDT_DIAG_LENGTH			GENMASK(7, 0)
+
+#define QCA808X_CDT_CODE_PAIR_A			GENMASK(15, 12)
+#define QCA808X_CDT_CODE_PAIR_B			GENMASK(11, 8)
+#define QCA808X_CDT_CODE_PAIR_C			GENMASK(7, 4)
+#define QCA808X_CDT_CODE_PAIR_D			GENMASK(3, 0)
+#define QCA808X_CDT_STATUS_STAT_FAIL		0
+#define QCA808X_CDT_STATUS_STAT_NORMAL		1
+#define QCA808X_CDT_STATUS_STAT_OPEN		2
+#define QCA808X_CDT_STATUS_STAT_SHORT		3
+
+/* QCA808X 1G chip type */
+#define QCA808X_PHY_MMD7_CHIP_TYPE		0x901d
+#define QCA808X_PHY_CHIP_TYPE_1G		BIT(0)
+
+#define QCA8081_PHY_SERDES_MMD1_FIFO_CTRL	0x9072
+#define QCA8081_PHY_FIFO_RSTN			BIT(11)
+
+#define QCA8081_PHY_ID				0x004dd101
+
+MODULE_DESCRIPTION("Qualcomm QCA808X PHY driver");
+MODULE_AUTHOR("Matus Ujhelyi");
+MODULE_LICENSE("GPL");
+
+static int qca808x_config_aneg(struct phy_device *phydev)
+{
+	int phy_ctrl = 0;
+	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;
+	}
+
+	/* Do not restart auto-negotiation by setting ret to 0 defautly,
+	 * when calling __genphy_config_aneg later.
+	 */
+	ret = 0;
+
+	/* The reg MII_BMCR also needs to be configured for force mode, the
+	 * genphy_config_aneg is also needed.
+	 */
+	if (phydev->autoneg == AUTONEG_DISABLE)
+		genphy_c45_pma_setup_forced(phydev);
+
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->advertising))
+		phy_ctrl = MDIO_AN_10GBT_CTRL_ADV2_5G;
+
+	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
+				     MDIO_AN_10GBT_CTRL_ADV2_5G, phy_ctrl);
+	if (ret < 0)
+		return ret;
+
+	return __genphy_config_aneg(phydev, ret);
+}
+
+static int qca808x_phy_fast_retrain_config(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Enable fast retrain */
+	ret = genphy_c45_fast_retrain(phydev, true);
+	if (ret)
+		return ret;
+
+	phy_write_mmd(phydev, MDIO_MMD_AN, QCA808X_PHY_MMD7_TOP_OPTION1,
+		      QCA808X_TOP_OPTION1_DATA);
+	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, QCA808X_PHY_MMD1_MSE_THRESHOLD_20DB,
+		      QCA808X_MSE_THRESHOLD_20DB_VALUE);
+	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, QCA808X_PHY_MMD1_MSE_THRESHOLD_17DB,
+		      QCA808X_MSE_THRESHOLD_17DB_VALUE);
+	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, QCA808X_PHY_MMD1_MSE_THRESHOLD_27DB,
+		      QCA808X_MSE_THRESHOLD_27DB_VALUE);
+	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, QCA808X_PHY_MMD1_MSE_THRESHOLD_28DB,
+		      QCA808X_MSE_THRESHOLD_28DB_VALUE);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_1,
+		      QCA808X_MMD3_DEBUG_1_VALUE);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_4,
+		      QCA808X_MMD3_DEBUG_4_VALUE);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_5,
+		      QCA808X_MMD3_DEBUG_5_VALUE);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_3,
+		      QCA808X_MMD3_DEBUG_3_VALUE);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_6,
+		      QCA808X_MMD3_DEBUG_6_VALUE);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_2,
+		      QCA808X_MMD3_DEBUG_2_VALUE);
+
+	return 0;
+}
+
+static int qca808x_phy_ms_seed_enable(struct phy_device *phydev, bool enable)
+{
+	u16 seed_value;
+
+	if (!enable)
+		return at803x_debug_reg_mask(phydev, QCA808X_PHY_DEBUG_LOCAL_SEED,
+				QCA808X_MASTER_SLAVE_SEED_ENABLE, 0);
+
+	seed_value = get_random_u32_below(QCA808X_MASTER_SLAVE_SEED_RANGE);
+	return at803x_debug_reg_mask(phydev, QCA808X_PHY_DEBUG_LOCAL_SEED,
+			QCA808X_MASTER_SLAVE_SEED_CFG | QCA808X_MASTER_SLAVE_SEED_ENABLE,
+			FIELD_PREP(QCA808X_MASTER_SLAVE_SEED_CFG, seed_value) |
+			QCA808X_MASTER_SLAVE_SEED_ENABLE);
+}
+
+static bool qca808x_is_prefer_master(struct phy_device *phydev)
+{
+	return (phydev->master_slave_get == MASTER_SLAVE_CFG_MASTER_FORCE) ||
+		(phydev->master_slave_get == MASTER_SLAVE_CFG_MASTER_PREFERRED);
+}
+
+static bool qca808x_has_fast_retrain_or_slave_seed(struct phy_device *phydev)
+{
+	return linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->supported);
+}
+
+static int qca808x_config_init(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Active adc&vga on 802.3az for the link 1000M and 100M */
+	ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_ADDR_CLD_CTRL7,
+			     QCA808X_8023AZ_AFE_CTRL_MASK, QCA808X_8023AZ_AFE_EN);
+	if (ret)
+		return ret;
+
+	/* Adjust the threshold on 802.3az for the link 1000M */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS,
+			    QCA808X_PHY_MMD3_AZ_TRAINING_CTRL,
+			    QCA808X_MMD3_AZ_TRAINING_VAL);
+	if (ret)
+		return ret;
+
+	if (qca808x_has_fast_retrain_or_slave_seed(phydev)) {
+		/* Config the fast retrain for the link 2500M */
+		ret = qca808x_phy_fast_retrain_config(phydev);
+		if (ret)
+			return ret;
+
+		ret = genphy_read_master_slave(phydev);
+		if (ret < 0)
+			return ret;
+
+		if (!qca808x_is_prefer_master(phydev)) {
+			/* Enable seed and configure lower ramdom seed to make phy
+			 * linked as slave mode.
+			 */
+			ret = qca808x_phy_ms_seed_enable(phydev, true);
+			if (ret)
+				return ret;
+		}
+	}
+
+	/* Configure adc threshold as 100mv for the link 10M */
+	return at803x_debug_reg_mask(phydev, QCA808X_PHY_DEBUG_ADC_THRESHOLD,
+				     QCA808X_ADC_THRESHOLD_MASK,
+				     QCA808X_ADC_THRESHOLD_100MV);
+}
+
+static int qca808x_read_status(struct phy_device *phydev)
+{
+	struct at803x_ss_mask ss_mask = { 0 };
+	int ret;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_STAT);
+	if (ret < 0)
+		return ret;
+
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->lp_advertising,
+			 ret & MDIO_AN_10GBT_STAT_LP2_5G);
+
+	ret = genphy_read_status(phydev);
+	if (ret)
+		return ret;
+
+	/* qca8081 takes the different bits for speed value from at803x */
+	ss_mask.speed_mask = QCA808X_SS_SPEED_MASK;
+	ss_mask.speed_shift = __bf_shf(QCA808X_SS_SPEED_MASK);
+	ret = at803x_read_specific_status(phydev, ss_mask);
+	if (ret < 0)
+		return ret;
+
+	if (phydev->link) {
+		if (phydev->speed == SPEED_2500)
+			phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
+		else
+			phydev->interface = PHY_INTERFACE_MODE_SGMII;
+	} else {
+		/* generate seed as a lower random value to make PHY linked as SLAVE easily,
+		 * except for master/slave configuration fault detected or the master mode
+		 * preferred.
+		 *
+		 * the reason for not putting this code into the function link_change_notify is
+		 * the corner case where the link partner is also the qca8081 PHY and the seed
+		 * value is configured as the same value, the link can't be up and no link change
+		 * occurs.
+		 */
+		if (qca808x_has_fast_retrain_or_slave_seed(phydev)) {
+			if (phydev->master_slave_state == MASTER_SLAVE_STATE_ERR ||
+			    qca808x_is_prefer_master(phydev)) {
+				qca808x_phy_ms_seed_enable(phydev, false);
+			} else {
+				qca808x_phy_ms_seed_enable(phydev, true);
+			}
+		}
+	}
+
+	return 0;
+}
+
+static int qca808x_soft_reset(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = genphy_soft_reset(phydev);
+	if (ret < 0)
+		return ret;
+
+	if (qca808x_has_fast_retrain_or_slave_seed(phydev))
+		ret = qca808x_phy_ms_seed_enable(phydev, true);
+
+	return ret;
+}
+
+static bool qca808x_cdt_fault_length_valid(int cdt_code)
+{
+	switch (cdt_code) {
+	case QCA808X_CDT_STATUS_STAT_SHORT:
+	case QCA808X_CDT_STATUS_STAT_OPEN:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static int qca808x_cable_test_result_trans(int cdt_code)
+{
+	switch (cdt_code) {
+	case QCA808X_CDT_STATUS_STAT_NORMAL:
+		return ETHTOOL_A_CABLE_RESULT_CODE_OK;
+	case QCA808X_CDT_STATUS_STAT_SHORT:
+		return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
+	case QCA808X_CDT_STATUS_STAT_OPEN:
+		return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
+	case QCA808X_CDT_STATUS_STAT_FAIL:
+	default:
+		return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
+	}
+}
+
+static int qca808x_cdt_fault_length(struct phy_device *phydev, int pair)
+{
+	int val;
+	u32 cdt_length_reg = 0;
+
+	switch (pair) {
+	case ETHTOOL_A_CABLE_PAIR_A:
+		cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_A;
+		break;
+	case ETHTOOL_A_CABLE_PAIR_B:
+		cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_B;
+		break;
+	case ETHTOOL_A_CABLE_PAIR_C:
+		cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_C;
+		break;
+	case ETHTOOL_A_CABLE_PAIR_D:
+		cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_D;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	val = phy_read_mmd(phydev, MDIO_MMD_PCS, cdt_length_reg);
+	if (val < 0)
+		return val;
+
+	return (FIELD_GET(QCA808X_CDT_DIAG_LENGTH, val) * 824) / 10;
+}
+
+static int qca808x_cable_test_start(struct phy_device *phydev)
+{
+	int ret;
+
+	/* perform CDT with the following configs:
+	 * 1. disable hibernation.
+	 * 2. force PHY working in MDI mode.
+	 * 3. for PHY working in 1000BaseT.
+	 * 4. configure the threshold.
+	 */
+
+	ret = at803x_debug_reg_mask(phydev, QCA808X_DBG_AN_TEST, QCA808X_HIBERNATION_EN, 0);
+	if (ret < 0)
+		return ret;
+
+	ret = at803x_config_mdix(phydev, ETH_TP_MDI);
+	if (ret < 0)
+		return ret;
+
+	/* Force 1000base-T needs to configure PMA/PMD and MII_BMCR */
+	phydev->duplex = DUPLEX_FULL;
+	phydev->speed = SPEED_1000;
+	ret = genphy_c45_pma_setup_forced(phydev);
+	if (ret < 0)
+		return ret;
+
+	ret = genphy_setup_forced(phydev);
+	if (ret < 0)
+		return ret;
+
+	/* configure the thresholds for open, short, pair ok test */
+	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x8074, 0xc040);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x8076, 0xc040);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x8077, 0xa060);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x8078, 0xc050);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x807a, 0xc060);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x807e, 0xb060);
+
+	return 0;
+}
+
+static int qca808x_cdt_start(struct phy_device *phydev)
+{
+	u16 cdt;
+
+	/* qca8081 takes the different bit 15 to enable CDT test */
+	cdt = QCA808X_CDT_ENABLE_TEST |
+	      QCA808X_CDT_LENGTH_UNIT |
+	      QCA808X_CDT_INTER_CHECK_DIS;
+
+	return phy_write(phydev, AT803X_CDT, cdt);
+}
+
+static int qca808x_cdt_wait_for_completition(struct phy_device *phydev)
+{
+	int val, ret;
+
+	/* One test run takes about 25ms */
+	ret = phy_read_poll_timeout(phydev, AT803X_CDT, val,
+				    !(val & QCA808X_CDT_ENABLE_TEST),
+				    30000, 100000, true);
+
+	return ret < 0 ? ret : 0;
+}
+
+static int qca808x_cable_test_get_status(struct phy_device *phydev, bool *finished)
+{
+	int ret, val;
+	int pair_a, pair_b, pair_c, pair_d;
+
+	*finished = false;
+
+	ret = qca808x_cdt_start(phydev);
+	if (ret)
+		return ret;
+
+	ret = qca808x_cdt_wait_for_completition(phydev);
+	if (ret)
+		return ret;
+
+	val = phy_read_mmd(phydev, MDIO_MMD_PCS, QCA808X_MMD3_CDT_STATUS);
+	if (val < 0)
+		return val;
+
+	pair_a = FIELD_GET(QCA808X_CDT_CODE_PAIR_A, val);
+	pair_b = FIELD_GET(QCA808X_CDT_CODE_PAIR_B, val);
+	pair_c = FIELD_GET(QCA808X_CDT_CODE_PAIR_C, val);
+	pair_d = FIELD_GET(QCA808X_CDT_CODE_PAIR_D, val);
+
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
+				qca808x_cable_test_result_trans(pair_a));
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_B,
+				qca808x_cable_test_result_trans(pair_b));
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_C,
+				qca808x_cable_test_result_trans(pair_c));
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_D,
+				qca808x_cable_test_result_trans(pair_d));
+
+	if (qca808x_cdt_fault_length_valid(pair_a))
+		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A,
+				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A));
+	if (qca808x_cdt_fault_length_valid(pair_b))
+		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B,
+				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B));
+	if (qca808x_cdt_fault_length_valid(pair_c))
+		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C,
+				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C));
+	if (qca808x_cdt_fault_length_valid(pair_d))
+		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D,
+				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D));
+
+	*finished = true;
+
+	return 0;
+}
+
+static int qca808x_get_features(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = genphy_c45_pma_read_abilities(phydev);
+	if (ret)
+		return ret;
+
+	/* The autoneg ability is not existed in bit3 of MMD7.1,
+	 * but it is supported by qca808x PHY, so we add it here
+	 * manually.
+	 */
+	linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported);
+
+	/* As for the qca8081 1G version chip, the 2500baseT ability is also
+	 * existed in the bit0 of MMD1.21, we need to remove it manually if
+	 * it is the qca8081 1G chip according to the bit0 of MMD7.0x901d.
+	 */
+	ret = phy_read_mmd(phydev, MDIO_MMD_AN, QCA808X_PHY_MMD7_CHIP_TYPE);
+	if (ret < 0)
+		return ret;
+
+	if (QCA808X_PHY_CHIP_TYPE_1G & ret)
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->supported);
+
+	return 0;
+}
+
+static void qca808x_link_change_notify(struct phy_device *phydev)
+{
+	/* Assert interface sgmii fifo on link down, deassert it on link up,
+	 * the interface device address is always phy address added by 1.
+	 */
+	mdiobus_c45_modify_changed(phydev->mdio.bus, phydev->mdio.addr + 1,
+				   MDIO_MMD_PMAPMD, QCA8081_PHY_SERDES_MMD1_FIFO_CTRL,
+				   QCA8081_PHY_FIFO_RSTN,
+				   phydev->link ? QCA8081_PHY_FIFO_RSTN : 0);
+}
+
+static struct phy_driver qca808x_driver[] = {
+{
+	/* Qualcomm QCA8081 */
+	PHY_ID_MATCH_EXACT(QCA8081_PHY_ID),
+	.name			= "Qualcomm QCA8081",
+	.flags			= PHY_POLL_CABLE_TEST,
+	.config_intr		= at803x_config_intr,
+	.handle_interrupt	= at803x_handle_interrupt,
+	.get_tunable		= at803x_get_tunable,
+	.set_tunable		= at803x_set_tunable,
+	.set_wol		= at803x_set_wol,
+	.get_wol		= at803x_get_wol,
+	.get_features		= qca808x_get_features,
+	.config_aneg		= qca808x_config_aneg,
+	.suspend		= genphy_suspend,
+	.resume			= genphy_resume,
+	.read_status		= qca808x_read_status,
+	.config_init		= qca808x_config_init,
+	.soft_reset		= qca808x_soft_reset,
+	.cable_test_start	= qca808x_cable_test_start,
+	.cable_test_get_status	= qca808x_cable_test_get_status,
+	.link_change_notify	= qca808x_link_change_notify,
+}, };
+
+module_phy_driver(qca808x_driver);
+
+static struct mdio_device_id __maybe_unused qca808x_tbl[] = {
+	{ PHY_ID_MATCH_EXACT(QCA8081_PHY_ID) },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(mdio, qca808x_tbl);
diff --git a/drivers/net/phy/qcom/qcom.h b/drivers/net/phy/qcom/qcom.h
index 17cc6705dd3e..cc968ea3e972 100644
--- a/drivers/net/phy/qcom/qcom.h
+++ b/drivers/net/phy/qcom/qcom.h
@@ -1,5 +1,65 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 
+#define AT803X_SPECIFIC_FUNCTION_CONTROL	0x10
+#define AT803X_SFC_ASSERT_CRS			BIT(11)
+#define AT803X_SFC_FORCE_LINK			BIT(10)
+#define AT803X_SFC_MDI_CROSSOVER_MODE_M		GENMASK(6, 5)
+#define AT803X_SFC_AUTOMATIC_CROSSOVER		0x3
+#define AT803X_SFC_MANUAL_MDIX			0x1
+#define AT803X_SFC_MANUAL_MDI			0x0
+#define AT803X_SFC_SQE_TEST			BIT(2)
+#define AT803X_SFC_POLARITY_REVERSAL		BIT(1)
+#define AT803X_SFC_DISABLE_JABBER		BIT(0)
+
+#define AT803X_SPECIFIC_STATUS			0x11
+#define AT803X_SS_SPEED_MASK			GENMASK(15, 14)
+#define AT803X_SS_SPEED_1000			2
+#define AT803X_SS_SPEED_100			1
+#define AT803X_SS_SPEED_10			0
+#define AT803X_SS_DUPLEX			BIT(13)
+#define AT803X_SS_SPEED_DUPLEX_RESOLVED		BIT(11)
+#define AT803X_SS_MDIX				BIT(6)
+
+#define QCA808X_SS_SPEED_MASK			GENMASK(9, 7)
+#define QCA808X_SS_SPEED_2500			4
+
+#define AT803X_INTR_ENABLE			0x12
+#define AT803X_INTR_ENABLE_AUTONEG_ERR		BIT(15)
+#define AT803X_INTR_ENABLE_SPEED_CHANGED	BIT(14)
+#define AT803X_INTR_ENABLE_DUPLEX_CHANGED	BIT(13)
+#define AT803X_INTR_ENABLE_PAGE_RECEIVED	BIT(12)
+#define AT803X_INTR_ENABLE_LINK_FAIL		BIT(11)
+#define AT803X_INTR_ENABLE_LINK_SUCCESS		BIT(10)
+#define AT803X_INTR_ENABLE_LINK_FAIL_BX		BIT(8)
+#define AT803X_INTR_ENABLE_LINK_SUCCESS_BX	BIT(7)
+#define AT803X_INTR_ENABLE_WIRESPEED_DOWNGRADE	BIT(5)
+#define AT803X_INTR_ENABLE_POLARITY_CHANGED	BIT(1)
+#define AT803X_INTR_ENABLE_WOL			BIT(0)
+
+#define AT803X_INTR_STATUS			0x13
+
+#define AT803X_SMART_SPEED			0x14
+#define AT803X_SMART_SPEED_ENABLE		BIT(5)
+#define AT803X_SMART_SPEED_RETRY_LIMIT_MASK	GENMASK(4, 2)
+#define AT803X_SMART_SPEED_BYPASS_TIMER		BIT(1)
+
+#define AT803X_CDT				0x16
+#define AT803X_CDT_MDI_PAIR_MASK		GENMASK(9, 8)
+#define AT803X_CDT_ENABLE_TEST			BIT(0)
+#define AT803X_CDT_STATUS			0x1c
+#define AT803X_CDT_STATUS_STAT_NORMAL		0
+#define AT803X_CDT_STATUS_STAT_SHORT		1
+#define AT803X_CDT_STATUS_STAT_OPEN		2
+#define AT803X_CDT_STATUS_STAT_FAIL		3
+#define AT803X_CDT_STATUS_STAT_MASK		GENMASK(9, 8)
+#define AT803X_CDT_STATUS_DELTA_TIME_MASK	GENMASK(7, 0)
+
+#define AT803X_PHY_MMD3_WOL_CTRL		0x8012
+#define AT803X_WOL_EN				BIT(5)
+#define AT803X_LOC_MAC_ADDR_0_15_OFFSET		0x804C
+#define AT803X_LOC_MAC_ADDR_16_31_OFFSET	0x804B
+#define AT803X_LOC_MAC_ADDR_32_47_OFFSET	0x804A
+
 #define AT803X_DEBUG_ANALOG_TEST_CTRL		0x00
 #define QCA8327_DEBUG_MANU_CTRL_EN		BIT(2)
 #define QCA8337_DEBUG_MANU_CTRL_EN		GENMASK(3, 2)
@@ -20,6 +80,10 @@
 #define AT803X_DEBUG_PLL_ON			BIT(2)
 #define AT803X_DEBUG_RGMII_1V8			BIT(3)
 
+#define AT803X_DEFAULT_DOWNSHIFT		5
+#define AT803X_MIN_DOWNSHIFT			2
+#define AT803X_MAX_DOWNSHIFT			9
+
 enum stat_access_type {
 	PHY,
 	MMD
@@ -32,7 +96,29 @@ struct at803x_hw_stat {
 	enum stat_access_type access_type;
 };
 
+struct at803x_ss_mask {
+	u16 speed_mask;
+	u8 speed_shift;
+};
+
 int at803x_debug_reg_read(struct phy_device *phydev, u16 reg);
 int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
 			  u16 clear, u16 set);
 int at803x_debug_reg_write(struct phy_device *phydev, u16 reg, u16 data);
+
+int at803x_config_mdix(struct phy_device *phydev, u8 ctrl);
+int at803x_read_specific_status(struct phy_device *phydev,
+				struct at803x_ss_mask ss_mask);
+int at803x_config_intr(struct phy_device *phydev);
+int at803x_ack_interrupt(struct phy_device *phydev);
+irqreturn_t at803x_handle_interrupt(struct phy_device *phydev);
+int at803x_get_tunable(struct phy_device *phydev,
+		       struct ethtool_tunable *tuna, void *data);
+int at803x_set_tunable(struct phy_device *phydev,
+		       struct ethtool_tunable *tuna, const void *data);
+int at803x_get_downshift(struct phy_device *phydev, u8 *d);
+int at803x_set_downshift(struct phy_device *phydev, u8 cnt);
+int at803x_set_wol(struct phy_device *phydev,
+		   struct ethtool_wolinfo *wol);
+void at803x_get_wol(struct phy_device *phydev,
+		    struct ethtool_wolinfo *wol);
-- 
2.40.1