[PATCH net-next 0/5] net: phy: Reduce duplication

Hi all,

This patch series reduces the duplication among 10G PHY drivers that just
essentially stub most functions, but do that while replicating what the existing
generic functions do.

Florian Fainelli (5):
  net: phy: aquantia: Utilize genphy_c45_aneg_done()
  net: phy: Export gen10g_* functions
  net: phy: teranetics: Utilize generic functions
  net: phy: cortina: Utilize generic functions
  net: phy: marvell10g: Utilize gen10g_soft_reset()

 drivers/net/phy/aquantia.c   | 20 ++++++--------------
 drivers/net/phy/cortina.c    | 18 +++---------------
 drivers/net/phy/marvell10g.c |  7 +------
 drivers/net/phy/phy-c45.c    | 18 ++++++++++++------
 drivers/net/phy/teranetics.c | 30 +++++-------------------------
 include/linux/phy.h          |  8 ++++++++
 6 files changed, 35 insertions(+), 66 deletions(-)

-- 
2.14.1

The driver duplicates what the generic function does, so use the generic
function intead.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/aquantia.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/net/phy/aquantia.c b/drivers/net/phy/aquantia.c
index e8ae50e1255e..319edc9c8ec7 100644
--- a/drivers/net/phy/aquantia.c
+++ b/drivers/net/phy/aquantia.c

@@ -38,14 +38,6 @@ static int aquantia_config_aneg(struct phy_device *phydev)
 	return 0;
 }
 
-static int aquantia_aneg_done(struct phy_device *phydev)
-{
-	int reg;
-
-	reg = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
-	return (reg < 0) ? reg : (reg & BMSR_ANEGCOMPLETE);
-}
-
 static int aquantia_config_intr(struct phy_device *phydev)
 {
 	int err;

@@ -125,7 +117,7 @@ static struct phy_driver aquantia_driver[] = {
 	.name		= "Aquantia AQ1202",
 	.features	= PHY_AQUANTIA_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT,
-	.aneg_done	= aquantia_aneg_done,
+	.aneg_done	= genphy_c45_aneg_done,
 	.config_aneg    = aquantia_config_aneg,
 	.config_intr	= aquantia_config_intr,
 	.ack_interrupt	= aquantia_ack_interrupt,

@@ -137,7 +129,7 @@ static struct phy_driver aquantia_driver[] = {
 	.name		= "Aquantia AQ2104",
 	.features	= PHY_AQUANTIA_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT,
-	.aneg_done	= aquantia_aneg_done,
+	.aneg_done	= genphy_c45_aneg_done,
 	.config_aneg    = aquantia_config_aneg,
 	.config_intr	= aquantia_config_intr,
 	.ack_interrupt	= aquantia_ack_interrupt,

@@ -149,7 +141,7 @@ static struct phy_driver aquantia_driver[] = {
 	.name		= "Aquantia AQR105",
 	.features	= PHY_AQUANTIA_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT,
-	.aneg_done	= aquantia_aneg_done,
+	.aneg_done	= genphy_c45_aneg_done,
 	.config_aneg    = aquantia_config_aneg,
 	.config_intr	= aquantia_config_intr,
 	.ack_interrupt	= aquantia_ack_interrupt,

@@ -161,7 +153,7 @@ static struct phy_driver aquantia_driver[] = {
 	.name		= "Aquantia AQR106",
 	.features	= PHY_AQUANTIA_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT,
-	.aneg_done	= aquantia_aneg_done,
+	.aneg_done	= genphy_c45_aneg_done,
 	.config_aneg    = aquantia_config_aneg,
 	.config_intr	= aquantia_config_intr,
 	.ack_interrupt	= aquantia_ack_interrupt,

@@ -173,7 +165,7 @@ static struct phy_driver aquantia_driver[] = {
 	.name		= "Aquantia AQR107",
 	.features	= PHY_AQUANTIA_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT,
-	.aneg_done	= aquantia_aneg_done,
+	.aneg_done	= genphy_c45_aneg_done,
 	.config_aneg    = aquantia_config_aneg,
 	.config_intr	= aquantia_config_intr,
 	.ack_interrupt	= aquantia_ack_interrupt,

@@ -185,7 +177,7 @@ static struct phy_driver aquantia_driver[] = {
 	.name		= "Aquantia AQR405",
 	.features	= PHY_AQUANTIA_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT,
-	.aneg_done	= aquantia_aneg_done,
+	.aneg_done	= genphy_c45_aneg_done,
 	.config_aneg    = aquantia_config_aneg,
 	.config_intr	= aquantia_config_intr,
 	.ack_interrupt	= aquantia_ack_interrupt,

-- 
2.14.1

Update teranetics_aneg_done() to use genphy_c45_aneg_done() instead of
duplicating that code, and switch to gen10g_* functions where
appropriate instead of maintaining identical copies doing nothing.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/teranetics.c | 30 +++++-------------------------
 1 file changed, 5 insertions(+), 25 deletions(-)

diff --git a/drivers/net/phy/teranetics.c b/drivers/net/phy/teranetics.c
index fb2cef764e9a..09f6829cfcc8 100644
--- a/drivers/net/phy/teranetics.c
+++ b/drivers/net/phy/teranetics.c

@@ -34,19 +34,6 @@ MODULE_LICENSE("GPL v2");
 				MDIO_PHYXS_LNSTAT_SYNC3 | \
 				MDIO_PHYXS_LNSTAT_ALIGN)
 
-static int teranetics_config_init(struct phy_device *phydev)
-{
-	phydev->supported = SUPPORTED_10000baseT_Full;
-	phydev->advertising = SUPPORTED_10000baseT_Full;
-
-	return 0;
-}
-
-static int teranetics_soft_reset(struct phy_device *phydev)
-{
-	return 0;
-}
-
 static int teranetics_aneg_done(struct phy_device *phydev)
 {
 	int reg;

@@ -54,19 +41,12 @@ static int teranetics_aneg_done(struct phy_device *phydev)
 	/* auto negotiation state can only be checked when using copper
 	 * port, if using fiber port, just lie it's done.
 	 */
-	if (!phy_read_mmd(phydev, MDIO_MMD_VEND1, 93)) {
-		reg = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
-		return (reg < 0) ? reg : (reg & BMSR_ANEGCOMPLETE);
-	}
+	if (!phy_read_mmd(phydev, MDIO_MMD_VEND1, 93))
+		return genphy_c45_aneg_done(phydev);
 
 	return 1;
 }
 
-static int teranetics_config_aneg(struct phy_device *phydev)
-{
-	return 0;
-}
-
 static int teranetics_read_status(struct phy_device *phydev)
 {
 	int reg;

@@ -102,10 +82,10 @@ static struct phy_driver teranetics_driver[] = {
 	.phy_id		= PHY_ID_TN2020,
 	.phy_id_mask	= 0xffffffff,
 	.name		= "Teranetics TN2020",
-	.soft_reset	= teranetics_soft_reset,
+	.soft_reset	= gen10g_soft_reset,
 	.aneg_done	= teranetics_aneg_done,
-	.config_init    = teranetics_config_init,
-	.config_aneg    = teranetics_config_aneg,
+	.config_init    = gen10g_config_init,
+	.config_aneg    = gen10g_config_aneg,
 	.read_status	= teranetics_read_status,
 	.match_phy_device = teranetics_match_phy_device,
 },

-- 
2.14.1

cortina_soft_reset() does the same thing as gen10g_soft_reset(), and
cortina_config_aneg() is actually doing what gen10g_config_init() does
for 10G capable PHYs.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/cortina.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/cortina.c b/drivers/net/phy/cortina.c
index 9442db221834..9afbeb8b3667 100644
--- a/drivers/net/phy/cortina.c
+++ b/drivers/net/phy/cortina.c

@@ -30,14 +30,6 @@ static int cortina_read_reg(struct phy_device *phydev, u16 regnum)
 			    MII_ADDR_C45 | regnum);
 }
 
-static int cortina_config_aneg(struct phy_device *phydev)
-{
-	phydev->supported = SUPPORTED_10000baseT_Full;
-	phydev->advertising = SUPPORTED_10000baseT_Full;
-
-	return 0;
-}
-
 static int cortina_read_status(struct phy_device *phydev)
 {
 	int gpio_int_status, ret = 0;

@@ -61,11 +53,6 @@ static int cortina_read_status(struct phy_device *phydev)
 	return ret;
 }
 
-static int cortina_soft_reset(struct phy_device *phydev)
-{
-	return 0;
-}
-
 static int cortina_probe(struct phy_device *phydev)
 {
 	u32 phy_id = 0;

@@ -101,9 +88,10 @@ static struct phy_driver cortina_driver[] = {
 	.phy_id		= PHY_ID_CS4340,
 	.phy_id_mask	= 0xffffffff,
 	.name		= "Cortina CS4340",
-	.config_aneg	= cortina_config_aneg,
+	.config_init	= gen10g_config_init,
+	.config_aneg	= gen10g_config_aneg,
 	.read_status	= cortina_read_status,
-	.soft_reset	= cortina_soft_reset,
+	.soft_reset	= gen10g_soft_reset,
 	.probe		= cortina_probe,
 },
 };

-- 
2.14.1

We do the same thing as the generic function: nothing, so utilize it.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/marvell10g.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 8a0bd98fdec7..da014eae1476 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c

@@ -75,11 +75,6 @@ static int mv3310_probe(struct phy_device *phydev)
  * Resetting the MV88X3310 causes it to become non-responsive.  Avoid
  * setting the reset bit(s).
  */
-static int mv3310_soft_reset(struct phy_device *phydev)
-{
-	return 0;
-}
-
 static int mv3310_config_init(struct phy_device *phydev)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = { 0, };

@@ -377,7 +372,7 @@ static struct phy_driver mv3310_drivers[] = {
 				  SUPPORTED_10000baseT_Full |
 				  SUPPORTED_Backplane,
 		.probe		= mv3310_probe,
-		.soft_reset	= mv3310_soft_reset,
+		.soft_reset	= gen10g_soft_reset,
 		.config_init	= mv3310_config_init,
 		.config_aneg	= mv3310_config_aneg,
 		.aneg_done	= mv3310_aneg_done,

-- 
2.14.1

In order to remove a fair amount of duplication in the different 10G PHY
drivers, export all gen10g_* functions to be able to make use of those.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy-c45.c | 18 ++++++++++++------
 include/linux/phy.h       |  8 ++++++++
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index a4576859afae..f882dfae8e41 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c

@@ -268,12 +268,13 @@ EXPORT_SYMBOL_GPL(genphy_c45_read_mdix);
 
 /* The gen10g_* functions are the old Clause 45 stub */
 
-static int gen10g_config_aneg(struct phy_device *phydev)
+int gen10g_config_aneg(struct phy_device *phydev)
 {
 	return 0;
 }
+EXPORT_SYMBOL_GPL(gen10g_config_aneg);
 
-static int gen10g_read_status(struct phy_device *phydev)
+int gen10g_read_status(struct phy_device *phydev)
 {
 	u32 mmd_mask = phydev->c45_ids.devices_in_package;
 	int ret;

@@ -291,14 +292,16 @@ static int gen10g_read_status(struct phy_device *phydev)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(gen10g_read_status);
 
-static int gen10g_soft_reset(struct phy_device *phydev)
+int gen10g_soft_reset(struct phy_device *phydev)
 {
 	/* Do nothing for now */
 	return 0;
 }
+EXPORT_SYMBOL_GPL(gen10g_soft_reset);
 
-static int gen10g_config_init(struct phy_device *phydev)
+int gen10g_config_init(struct phy_device *phydev)
 {
 	/* Temporarily just say we support everything */
 	phydev->supported = SUPPORTED_10000baseT_Full;

@@ -306,16 +309,19 @@ static int gen10g_config_init(struct phy_device *phydev)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(gen10g_config_init);
 
-static int gen10g_suspend(struct phy_device *phydev)
+int gen10g_suspend(struct phy_device *phydev)
 {
 	return 0;
 }
+EXPORT_SYMBOL_GPL(gen10g_suspend);
 
-static int gen10g_resume(struct phy_device *phydev)
+int gen10g_resume(struct phy_device *phydev)
 {
 	return 0;
 }
+EXPORT_SYMBOL_GPL(gen10g_resume);
 
 struct phy_driver genphy_10g_driver = {
 	.phy_id         = 0xffffffff,

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5a0c3e53e7c2..612637e7584a 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -994,6 +994,14 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev);
 int genphy_c45_an_disable_aneg(struct phy_device *phydev);
 int genphy_c45_read_mdix(struct phy_device *phydev);
 
+/* The gen10g_* functions are the old Clause 45 stub */
+int gen10g_config_aneg(struct phy_device *phydev);
+int gen10g_read_status(struct phy_device *phydev);
+int gen10g_soft_reset(struct phy_device *phydev);
+int gen10g_config_init(struct phy_device *phydev);
+int gen10g_suspend(struct phy_device *phydev);
+int gen10g_resume(struct phy_device *phydev);
+
 static inline int phy_read_status(struct phy_device *phydev)
 {
 	if (!phydev->drv)

-- 
2.14.1

On Wed, Feb 28, 2018 at 11:36:09AM -0800, Florian Fainelli wrote:

In order to remove a fair amount of duplication in the different 10G PHY
drivers, export all gen10g_* functions to be able to make use of those.

The gen10g functions tend to be barely functional - for example,
gen10g_soft_reset() doesn't do the soft-reset thing, which could be
argued to be a bug, as there's a generic way to do that.

However, doing that for the Marvell 10G driver would be a big mistake.
The PHY crashes if you try to set the reset bit in any of the control
register 1s.

-- 
Russell King

On Wed, Feb 28, 2018 at 11:36:12AM -0800, Florian Fainelli wrote:

You do realise that getting rid of that function makes a nonsense of the
comment above it - and removing the comment along with the function gets
rid of the very important reason _why_ we have en empty reset method?

-- 
Russell King
ARM architecture Linux Kernel maintainer

Florian,

On Wed, Feb 28, 2018 at 11:44 AM, Russell King [off-list ref] wrote:

On Wed, Feb 28, 2018 at 11:36:12AM -0800, Florian Fainelli wrote:

quoted

We do the same thing as the generic function: nothing, so utilize it.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/marvell10g.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 8a0bd98fdec7..da014eae1476 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c

@@ -75,11 +75,6 @@ static int mv3310_probe(struct phy_device *phydev)
  * Resetting the MV88X3310 causes it to become non-responsive.  Avoid
  * setting the reset bit(s).
  */
-static int mv3310_soft_reset(struct phy_device *phydev)
-{
-     return 0;
-}
-

You do realise that getting rid of that function makes a nonsense of the
comment above it - and removing the comment along with the function gets
rid of the very important reason _why_ we have en empty reset method?

quoted

 static int mv3310_config_init(struct phy_device *phydev)
 {
      __ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = { 0, };

@@ -377,7 +372,7 @@ static struct phy_driver mv3310_drivers[] = {
                                SUPPORTED_10000baseT_Full |
                                SUPPORTED_Backplane,
              .probe          = mv3310_probe,
-             .soft_reset     = mv3310_soft_reset,
+             .soft_reset     = gen10g_soft_reset,
              .config_init    = mv3310_config_init,
              .config_aneg    = mv3310_config_aneg,
              .aneg_done      = mv3310_aneg_done,
--

2.14.1

--
Russell King
ARM architecture Linux Kernel maintainer

FWIW I have a local patch that goes something like that, which I meant to send
at one point and forgot

Something like that:

 static int gen10g_soft_reset(struct phy_device *phydev)
 {
+       int val;
+
+       val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1);
+       if (val < 0)
+               return val;
+
+       val |= MDIO_CTRL1_RESET;
+       phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, val);
+
 -       /* Do nothing for now */
        return 0;
 }

If that looks reasonable I can properly submit a patch,

Moritz

On 02/28/2018 11:49 AM, Moritz Fischer wrote:

Florian,

On Wed, Feb 28, 2018 at 11:44 AM, Russell King [off-list ref] wrote:

quoted

On Wed, Feb 28, 2018 at 11:36:12AM -0800, Florian Fainelli wrote:

quoted

We do the same thing as the generic function: nothing, so utilize it.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/marvell10g.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 8a0bd98fdec7..da014eae1476 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c

@@ -75,11 +75,6 @@ static int mv3310_probe(struct phy_device *phydev)
  * Resetting the MV88X3310 causes it to become non-responsive.  Avoid
  * setting the reset bit(s).
  */
-static int mv3310_soft_reset(struct phy_device *phydev)
-{
-     return 0;
-}
-

You do realise that getting rid of that function makes a nonsense of the
comment above it - and removing the comment along with the function gets
rid of the very important reason _why_ we have en empty reset method?

quoted

 static int mv3310_config_init(struct phy_device *phydev)
 {
      __ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = { 0, };

@@ -377,7 +372,7 @@ static struct phy_driver mv3310_drivers[] = {
                                SUPPORTED_10000baseT_Full |
                                SUPPORTED_Backplane,
              .probe          = mv3310_probe,
-             .soft_reset     = mv3310_soft_reset,
+             .soft_reset     = gen10g_soft_reset,
              .config_init    = mv3310_config_init,
              .config_aneg    = mv3310_config_aneg,
              .aneg_done      = mv3310_aneg_done,
--

2.14.1

--
Russell King
ARM architecture Linux Kernel maintainer

FWIW I have a local patch that goes something like that, which I meant to send
at one point and forgot

Something like that:

 static int gen10g_soft_reset(struct phy_device *phydev)
 {
+       int val;
+
+       val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1);
+       if (val < 0)
+               return val;
+
+       val |= MDIO_CTRL1_RESET;
+       phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, val);
+
 -       /* Do nothing for now */
        return 0;
 }

If that looks reasonable I can properly submit a patch,

Sure, I would have to check the specification document to make sure
whether this does not need to be coded similarly to what
genphy_soft_reset() does, which is polling for the reset bit to clear
itself.

v2 also renamed the current gen10g_soft_reset() into
gen10g_no_soft_reset() so your implementation makes even more sense now.
-- 
Florian