[PATCH net-next 0/4] net: phy: add and use genphy_c45_an_config_an

[PATCH net-next 0/4] net: phy: add and use genphy_c45_an_config_an

[Heiner Kallweit]

This series adds genphy_c45_an_config_an() and uses it in the
marvell10g diver. In addition patch 4 aligns the aneg configuration
with what is done in genphy_config_aneg().

Heiner Kallweit (4):
  net: phy: add helper linkmode_adv_to_mii_10gbt_adv_t
  net: phy: add genphy_c45_an_config_an
  net: phy: marvell10g: use genphy_c45_an_config_an
  net: phy: marvell10g: check for newly set aneg in mv3310_config_aneg

 drivers/net/phy/marvell10g.c | 28 ++++++++---------------
 drivers/net/phy/phy-c45.c    | 44 ++++++++++++++++++++++++++++++++++++
 include/linux/mdio.h         | 25 ++++++++++++++++++++
 include/linux/phy.h          |  1 +
 4 files changed, 79 insertions(+), 19 deletions(-)

-- 
2.20.1

[PATCH net-next 1/4] net: phy: add helper linkmode_adv_to_mii_10gbt_adv_t

[Heiner Kallweit]

Add a helper linkmode_adv_to_mii_10gbt_adv_t(), similar to
linkmode_adv_to_mii_adv_t.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 include/linux/mdio.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 5b872c45f..5a65f32d8 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -280,6 +280,31 @@ static inline void mii_10gbt_stat_mod_linkmode_lpa_t(unsigned long *advertising,
 			 advertising, lpa & MDIO_AN_10GBT_STAT_LP10G);
 }
 
+/**
+ * linkmode_adv_to_mii_10gbt_adv_t
+ * @advertising: the linkmode advertisement settings
+ *
+ * A small helper function that translates linkmode advertisement
+ * settings to phy autonegotiation advertisements for the C45
+ * 10GBASE-T AN CONTROL (7.32) register.
+ */
+static inline u32 linkmode_adv_to_mii_10gbt_adv_t(unsigned long *advertising)
+{
+	u32 result = 0;
+
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
+			      advertising))
+		result |= MDIO_AN_10GBT_CTRL_ADV2_5G;
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+			      advertising))
+		result |= MDIO_AN_10GBT_CTRL_ADV5G;
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
+			      advertising))
+		result |= MDIO_AN_10GBT_CTRL_ADV10G;
+
+	return result;
+}
+
 int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum);
 int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val);
 
-- 
2.20.1

Re: [PATCH net-next 1/4] net: phy: add helper linkmode_adv_to_mii_10gbt_adv_t

[Florian Fainelli]

On 2/16/2019 11:50 AM, Heiner Kallweit wrote:
> Add a helper linkmode_adv_to_mii_10gbt_adv_t(), similar to
> linkmode_adv_to_mii_adv_t.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH net-next 2/4] net: phy: add genphy_c45_an_config_an

[Heiner Kallweit]

From: Andrew Lunn <andrew@lunn.ch>
C45 configuration of 10/100 and multi-giga bit auto negotiation
advertisement is standardized. Configuration of 1000Base-T however
appears to be vendor specific. Move the generic code out of the
Marvell driver into the common phy-c45.c file.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
[hkallweit1@gmail.com: use new helper linkmode_adv_to_mii_10gbt_adv_t and split patch]
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy-c45.c | 44 +++++++++++++++++++++++++++++++++++++++
 include/linux/phy.h       |  1 +
 2 files changed, 45 insertions(+)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 0374c50b1..bea1b0c6e 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -78,6 +78,50 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
 }
 EXPORT_SYMBOL_GPL(genphy_c45_pma_setup_forced);
 
+/**
+ * genphy_c45_an_config_an - configure advertisement registers
+ * @phydev: target phy_device struct
+ *
+ * Configure advertisement registers based on modes set in phydev->advertising
+ *
+ * Returns negative errno code on failure, 0 if advertisement didn't change,
+ * or 1 if advertised modes changed.
+ */
+int genphy_c45_an_config_an(struct phy_device *phydev)
+{
+	int changed = 0, ret;
+	u32 adv;
+
+	linkmode_and(phydev->advertising, phydev->advertising,
+		     phydev->supported);
+
+	adv = linkmode_adv_to_mii_adv_t(phydev->advertising);
+
+	ret = phy_modify_mmd(phydev, MDIO_MMD_AN, MDIO_AN_ADVERTISE,
+			     ADVERTISE_ALL | ADVERTISE_100BASE4 |
+			     ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM,
+			     adv);
+	if (ret < 0)
+		return ret;
+	if (ret > 0)
+		changed = 1;
+
+	adv = linkmode_adv_to_mii_10gbt_adv_t(phydev->advertising);
+
+	ret = phy_modify_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
+			     MDIO_AN_10GBT_CTRL_ADV10G |
+			     MDIO_AN_10GBT_CTRL_ADV5G |
+			     MDIO_AN_10GBT_CTRL_ADV2_5G,
+			     adv);
+	if (ret < 0)
+		return ret;
+	if (ret > 0)
+		changed = 1;
+
+	return changed;
+}
+EXPORT_SYMBOL_GPL(genphy_c45_an_config_an);
+
 /**
  * genphy_c45_an_disable_aneg - disable auto-negotiation
  * @phydev: target phy_device struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index bf1070c2a..dabcef5ba 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1101,6 +1101,7 @@ int genphy_c45_read_link(struct phy_device *phydev);
 int genphy_c45_read_lpa(struct phy_device *phydev);
 int genphy_c45_read_pma(struct phy_device *phydev);
 int genphy_c45_pma_setup_forced(struct phy_device *phydev);
+int genphy_c45_an_config_an(struct phy_device *phydev);
 int genphy_c45_an_disable_aneg(struct phy_device *phydev);
 int genphy_c45_read_mdix(struct phy_device *phydev);
 int genphy_c45_pma_read_abilities(struct phy_device *phydev);
-- 
2.20.1

Re: [PATCH net-next 2/4] net: phy: add genphy_c45_an_config_an

[Florian Fainelli]

On 2/16/2019 11:51 AM, Heiner Kallweit wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> C45 configuration of 10/100 and multi-giga bit auto negotiation
> advertisement is standardized. Configuration of 1000Base-T however
> appears to be vendor specific. Move the generic code out of the
> Marvell driver into the common phy-c45.c file.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> [hkallweit1@gmail.com: use new helper linkmode_adv_to_mii_10gbt_adv_t and split patch]
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/phy/phy-c45.c | 44 +++++++++++++++++++++++++++++++++++++++
>  include/linux/phy.h       |  1 +
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index 0374c50b1..bea1b0c6e 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -78,6 +78,50 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL_GPL(genphy_c45_pma_setup_forced);
>  
> +/**
> + * genphy_c45_an_config_an - configure advertisement registers

Nit: are not the two "an" redundant" here? Unless the first one means
something different in which case naming this:
genphy_c45_an_config_aneg() would be clearer?


> + * @phydev: target phy_device struct
> + *
> + * Configure advertisement registers based on modes set in phydev->advertising
> + *
> + * Returns negative errno code on failure, 0 if advertisement didn't change,
> + * or 1 if advertised modes changed.
> + */
> +int genphy_c45_an_config_an(struct phy_device *phydev)
> +{
> +	int changed = 0, ret;
> +	u32 adv;
> +
> +	linkmode_and(phydev->advertising, phydev->advertising,
> +		     phydev->supported);
> +
> +	adv = linkmode_adv_to_mii_adv_t(phydev->advertising);
> +
> +	ret = phy_modify_mmd(phydev, MDIO_MMD_AN, MDIO_AN_ADVERTISE,
> +			     ADVERTISE_ALL | ADVERTISE_100BASE4 |
> +			     ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM,
> +			     adv);
> +	if (ret < 0)
> +		return ret;
> +	if (ret > 0)
> +		changed = 1;

'changed' is essentially a short hand for 'ret >= 0' given it is
initialized to 0 by default and only assigned 1 if 'ret > 0', and since
you do early returns in case 'ret < 0', you might as well drop 'changed'
entirely?

Other than that and the naming:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 2/4] net: phy: add genphy_c45_an_config_an

[Andrew Lunn]

On Sat, Feb 16, 2019 at 06:44:24PM -0800, Florian Fainelli wrote:
> 
> 
> On 2/16/2019 11:51 AM, Heiner Kallweit wrote:
> > From: Andrew Lunn <andrew@lunn.ch>
> > C45 configuration of 10/100 and multi-giga bit auto negotiation
> > advertisement is standardized. Configuration of 1000Base-T however
> > appears to be vendor specific. Move the generic code out of the
> > Marvell driver into the common phy-c45.c file.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > [hkallweit1@gmail.com: use new helper linkmode_adv_to_mii_10gbt_adv_t and split patch]
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > ---
> >  drivers/net/phy/phy-c45.c | 44 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/phy.h       |  1 +
> >  2 files changed, 45 insertions(+)
> > 
> > diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> > index 0374c50b1..bea1b0c6e 100644
> > --- a/drivers/net/phy/phy-c45.c
> > +++ b/drivers/net/phy/phy-c45.c
> > @@ -78,6 +78,50 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
> >  }
> >  EXPORT_SYMBOL_GPL(genphy_c45_pma_setup_forced);
> >  
> > +/**
> > + * genphy_c45_an_config_an - configure advertisement registers
> 
> Nit: are not the two "an" redundant" here? Unless the first one means
> something different in which case naming this:
> genphy_c45_an_config_aneg() would be clearer?

Hi Florian

Heiner and I had a brief discussion about this. There is a general
trend in the naming to have the device name in the function name.  So
genphy_c45_pma_setup_forced uses the PMA device registers,
genphy_c45_an_disable_aneg uses the AN device registers, etc.

However, genphy_c45_an_config_aneg() might be better.

	 Andrew

Re: [PATCH net-next 2/4] net: phy: add genphy_c45_an_config_an

[Heiner Kallweit]

On 17.02.2019 05:18, Andrew Lunn wrote:
> On Sat, Feb 16, 2019 at 06:44:24PM -0800, Florian Fainelli wrote:
>>
>>
>> On 2/16/2019 11:51 AM, Heiner Kallweit wrote:
>>> From: Andrew Lunn <andrew@lunn.ch>
>>> C45 configuration of 10/100 and multi-giga bit auto negotiation
>>> advertisement is standardized. Configuration of 1000Base-T however
>>> appears to be vendor specific. Move the generic code out of the
>>> Marvell driver into the common phy-c45.c file.
>>>
>>> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>>> [hkallweit1@gmail.com: use new helper linkmode_adv_to_mii_10gbt_adv_t and split patch]
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>  drivers/net/phy/phy-c45.c | 44 +++++++++++++++++++++++++++++++++++++++
>>>  include/linux/phy.h       |  1 +
>>>  2 files changed, 45 insertions(+)
>>>
>>> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
>>> index 0374c50b1..bea1b0c6e 100644
>>> --- a/drivers/net/phy/phy-c45.c
>>> +++ b/drivers/net/phy/phy-c45.c
>>> @@ -78,6 +78,50 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
>>>  }
>>>  EXPORT_SYMBOL_GPL(genphy_c45_pma_setup_forced);
>>>  
>>> +/**
>>> + * genphy_c45_an_config_an - configure advertisement registers
>>
>> Nit: are not the two "an" redundant" here? Unless the first one means
>> something different in which case naming this:
>> genphy_c45_an_config_aneg() would be clearer?
> 
> Hi Florian
> 
> Heiner and I had a brief discussion about this. There is a general
> trend in the naming to have the device name in the function name.  So
> genphy_c45_pma_setup_forced uses the PMA device registers,
> genphy_c45_an_disable_aneg uses the AN device registers, etc.
> 
> However, genphy_c45_an_config_aneg() might be better.
> 
Good, then I'll prepare an update with the changed name.

> 	 Andrew
> 
Heiner

[PATCH net-next 3/4] net: phy: marvell10g: use genphy_c45_an_config_an

[Heiner Kallweit]

From: Andrew Lunn <andrew@lunn.ch>
Use new function genphy_c45_config_aneg() in mv3310_config_aneg().

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
[hkallweit1@gmail.com: patch splitted]
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/marvell10g.c | 22 +---------------------
 1 file changed, 1 insertion(+), 21 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 4a6ae63ab..03fa50087 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -274,13 +274,7 @@ static int mv3310_config_aneg(struct phy_device *phydev)
 	if (phydev->autoneg == AUTONEG_DISABLE)
 		return genphy_c45_pma_setup_forced(phydev);
 
-	linkmode_and(phydev->advertising, phydev->advertising,
-		     phydev->supported);
-
-	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_ADVERTISE,
-			     ADVERTISE_ALL | ADVERTISE_100BASE4 |
-			     ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM,
-			     linkmode_adv_to_mii_adv_t(phydev->advertising));
+	ret = genphy_c45_an_config_an(phydev);
 	if (ret < 0)
 		return ret;
 	if (ret > 0)
@@ -294,20 +288,6 @@ static int mv3310_config_aneg(struct phy_device *phydev)
 	if (ret > 0)
 		changed = true;
 
-	/* 10G control register */
-	if (linkmode_test_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
-			      phydev->advertising))
-		reg = MDIO_AN_10GBT_CTRL_ADV10G;
-	else
-		reg = 0;
-
-	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
-				     MDIO_AN_10GBT_CTRL_ADV10G, reg);
-	if (ret < 0)
-		return ret;
-	if (ret > 0)
-		changed = true;
-
 	if (changed)
 		ret = genphy_c45_restart_aneg(phydev);
 
-- 
2.20.1

Re: [PATCH net-next 3/4] net: phy: marvell10g: use genphy_c45_an_config_an

[Florian Fainelli]

On 2/16/2019 11:52 AM, Heiner Kallweit wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Use new function genphy_c45_config_aneg() in mv3310_config_aneg().
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> [hkallweit1@gmail.com: patch splitted]
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/phy/marvell10g.c | 22 +---------------------
>  1 file changed, 1 insertion(+), 21 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index 4a6ae63ab..03fa50087 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -274,13 +274,7 @@ static int mv3310_config_aneg(struct phy_device *phydev)
>  	if (phydev->autoneg == AUTONEG_DISABLE)
>  		return genphy_c45_pma_setup_forced(phydev);
>  
> -	linkmode_and(phydev->advertising, phydev->advertising,
> -		     phydev->supported);
> -
> -	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_ADVERTISE,
> -			     ADVERTISE_ALL | ADVERTISE_100BASE4 |
> -			     ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM,
> -			     linkmode_adv_to_mii_adv_t(phydev->advertising));
> +	ret = genphy_c45_an_config_an(phydev);
>  	if (ret < 0)
>  		return ret;
>  	if (ret > 0)
> @@ -294,20 +288,6 @@ static int mv3310_config_aneg(struct phy_device *phydev)
>  	if (ret > 0)
>  		changed = true;
>  
> -	/* 10G control register */
> -	if (linkmode_test_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
> -			      phydev->advertising))
> -		reg = MDIO_AN_10GBT_CTRL_ADV10G;
> -	else
> -		reg = 0;

This is not strictly equivalent though because marvell10g only checks
for the 10000baseT_Full bit set whereas genphy_c45_an_config_an() calls
 linkmode_adv_to_mii_10gbt_adv_t() which you recently updated to also
check for 2.5G and 5G. This sounds about the right decision, but I
wonder if Russell did this for a reason (like not able to test 2500baseT
and 5000baseT?)

> -
> -	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
> -				     MDIO_AN_10GBT_CTRL_ADV10G, reg);
> -	if (ret < 0)
> -		return ret;
> -	if (ret > 0)
> -		changed = true;
> -
>  	if (changed)
>  		ret = genphy_c45_restart_aneg(phydev);
>  
> 

-- 
Florian

Re: [PATCH net-next 3/4] net: phy: marvell10g: use genphy_c45_an_config_an

[Andrew Lunn]

> This is not strictly equivalent though because marvell10g only checks
> for the 10000baseT_Full bit set whereas genphy_c45_an_config_an() calls
>  linkmode_adv_to_mii_10gbt_adv_t() which you recently updated to also
> check for 2.5G and 5G. This sounds about the right decision, but I
> wonder if Russell did this for a reason (like not able to test 2500baseT
> and 5000baseT?)

Hi Florian

Correct. But both Marvell MAC drivers recently gained the code needed
for 2500BaseX and 5000BaseX. See Maxime and Russell comphy patches.
Russell has tested 2500BaseX SFPs and Maxime has been testing
2500BaseT copper.

	  Andrew

[PATCH net-next 4/4] net: phy: marvell10g: check for newly set aneg in mv3310_config_aneg

[Heiner Kallweit]

Even if the advertisement registers content didn't change, we may have
just switched to aneg, and therefore have to trigger an aneg restart.
This matches the behavior of genphy_config_aneg().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/marvell10g.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 03fa50087..4d8a290eb 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -288,6 +288,16 @@ static int mv3310_config_aneg(struct phy_device *phydev)
 	if (ret > 0)
 		changed = true;
 
+	if (!changed) {
+		/* Configure and restart aneg if it wasn't set before */
+		ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1);
+		if (ret < 0)
+			return ret;
+
+		if (!(ret & MDIO_AN_CTRL1_ENABLE))
+			changed = 1;
+	}
+
 	if (changed)
 		ret = genphy_c45_restart_aneg(phydev);
 
-- 
2.20.1

Re: [PATCH net-next 4/4] net: phy: marvell10g: check for newly set aneg in mv3310_config_aneg

[Florian Fainelli]

On 2/16/2019 11:53 AM, Heiner Kallweit wrote:
> Even if the advertisement registers content didn't change, we may have
> just switched to aneg, and therefore have to trigger an aneg restart.
> This matches the behavior of genphy_config_aneg().
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian