[PATCH net-next 0/3] net: phy: let phy_speed_down/up support speeds >1Gbps

[PATCH net-next 0/3] net: phy: let phy_speed_down/up support speeds >1Gbps

[Heiner Kallweit]

So far phy_speed_down/up can be used up to 1Gbps only. Remove this
restriction and add needed helpers to phy-core.c

Heiner Kallweit (3):
  net: phy: add __set_linkmode_max_speed
  net: phy: add __phy_speed_down and phy_resolve_min_speed
  net: phy: let phy_speed_down/up support speeds >1Gbps

 drivers/net/phy/phy-core.c | 39 +++++++++++++++++++++++--
 drivers/net/phy/phy.c      | 60 ++++++++++----------------------------
 include/linux/phy.h        |  3 ++
 3 files changed, 56 insertions(+), 46 deletions(-)

-- 
2.22.0

[PATCH net-next 1/3] net: phy: add __set_linkmode_max_speed

[Heiner Kallweit]

We will need the functionality of __set_linkmode_max_speed also for
linkmode bitmaps other than phydev->supported. Therefore split it.

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

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 9ae3abb2d..de085f255 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -207,14 +207,15 @@ size_t phy_speeds(unsigned int *speeds, size_t size,
 	return count;
 }
 
-static int __set_phy_supported(struct phy_device *phydev, u32 max_speed)
+static int __set_linkmode_max_speed(struct phy_device *phydev, u32 max_speed,
+				    unsigned long *addr)
 {
 	const struct phy_setting *p;
 	int i;
 
 	for (i = 0, p = settings; i < ARRAY_SIZE(settings); i++, p++) {
 		if (p->speed > max_speed)
-			linkmode_clear_bit(p->bit, phydev->supported);
+			linkmode_clear_bit(p->bit, addr);
 		else
 			break;
 	}
@@ -222,6 +223,11 @@ static int __set_phy_supported(struct phy_device *phydev, u32 max_speed)
 	return 0;
 }
 
+static int __set_phy_supported(struct phy_device *phydev, u32 max_speed)
+{
+	return __set_linkmode_max_speed(phydev, max_speed, phydev->supported);
+}
+
 int phy_set_max_speed(struct phy_device *phydev, u32 max_speed)
 {
 	int err;
-- 
2.22.0

[PATCH net-next 2/3] net: phy: add __phy_speed_down and phy_resolve_min_speed

[Heiner Kallweit]

__phy_speed_down provides most of the functionality for phy_speed_down.
It makes use of new helper phy_resolve_min_speed that is based on the
sorting of the settings[] array.
In certain cases it may be helpful to be able to exclude legacy half
duplex modes, therefore prepare phy_resolve_min_speed() for it.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy-core.c | 29 +++++++++++++++++++++++++++++
 include/linux/phy.h        |  1 +
 2 files changed, 30 insertions(+)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index de085f255..d7331875e 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -316,6 +316,35 @@ void phy_resolve_aneg_linkmode(struct phy_device *phydev)
 }
 EXPORT_SYMBOL_GPL(phy_resolve_aneg_linkmode);
 
+static int phy_resolve_min_speed(struct phy_device *phydev, bool fdx_only)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
+	int i = ARRAY_SIZE(settings);
+
+	linkmode_and(common, phydev->lp_advertising, phydev->advertising);
+
+	while (i--) {
+		if (test_bit(settings[i].bit, common)) {
+			if (fdx_only && settings[i].duplex != DUPLEX_FULL)
+				continue;
+			return settings[i].speed;
+		}
+	}
+
+	return SPEED_UNKNOWN;
+}
+
+int __phy_speed_down(struct phy_device *phydev)
+{
+	int min_common_speed = phy_resolve_min_speed(phydev, true);
+
+	if (min_common_speed == SPEED_UNKNOWN)
+		return -EINVAL;
+
+	return __set_linkmode_max_speed(phydev, min_common_speed,
+					phydev->advertising);
+}
+
 static void mmd_phy_indirect(struct mii_bus *bus, int phy_addr, int devad,
 			     u16 regnum)
 {
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 781f4810c..4be6d3b47 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -665,6 +665,7 @@ size_t phy_speeds(unsigned int *speeds, size_t size,
 		  unsigned long *mask);
 void of_set_phy_supported(struct phy_device *phydev);
 void of_set_phy_eee_broken(struct phy_device *phydev);
+int __phy_speed_down(struct phy_device *phydev);
 
 /**
  * phy_is_started - Convenience function to check whether PHY is started
-- 
2.22.0

[PATCH net-next 3/3] net: phy: let phy_speed_down/up support speeds >1Gbps

[Heiner Kallweit]

So far phy_speed_down/up can be used up to 1Gbps only. Remove this
restriction by using new helper __phy_speed_down. New member adv_old
in struct phy_device is used by phy_speed_up to restore the advertised
modes before calling phy_speed_down. Don't simply advertise what is
supported because a user may have intentionally removed modes from
advertisement.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c | 60 ++++++++++++-------------------------------
 include/linux/phy.h   |  2 ++
 2 files changed, 18 insertions(+), 44 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index ef7aa738e..f8e68b3b4 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -608,38 +608,21 @@ static int phy_poll_aneg_done(struct phy_device *phydev)
  */
 int phy_speed_down(struct phy_device *phydev, bool sync)
 {
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv_old);
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv);
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv_tmp);
 	int ret;
 
 	if (phydev->autoneg != AUTONEG_ENABLE)
 		return 0;
 
-	linkmode_copy(adv_old, phydev->advertising);
-	linkmode_copy(adv, phydev->lp_advertising);
-	linkmode_and(adv, adv, phydev->supported);
-
-	if (linkmode_test_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, adv) ||
-	    linkmode_test_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, adv)) {
-		linkmode_clear_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
-				   phydev->advertising);
-		linkmode_clear_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
-				   phydev->advertising);
-		linkmode_clear_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
-				   phydev->advertising);
-		linkmode_clear_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
-				   phydev->advertising);
-	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
-				     adv) ||
-		   linkmode_test_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
-				     adv)) {
-		linkmode_clear_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
-				   phydev->advertising);
-		linkmode_clear_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
-				   phydev->advertising);
-	}
+	linkmode_copy(adv_tmp, phydev->advertising);
+
+	ret = __phy_speed_down(phydev);
+	if (ret)
+		return ret;
 
-	if (linkmode_equal(phydev->advertising, adv_old))
+	linkmode_copy(phydev->adv_old, adv_tmp);
+
+	if (linkmode_equal(phydev->advertising, adv_tmp))
 		return 0;
 
 	ret = phy_config_aneg(phydev);
@@ -658,30 +641,19 @@ EXPORT_SYMBOL_GPL(phy_speed_down);
  */
 int phy_speed_up(struct phy_device *phydev)
 {
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(all_speeds) = { 0, };
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(not_speeds);
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv_old);
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(speeds);
-
-	linkmode_copy(adv_old, phydev->advertising);
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv_tmp);
 
 	if (phydev->autoneg != AUTONEG_ENABLE)
 		return 0;
 
-	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, all_speeds);
-	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, all_speeds);
-	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, all_speeds);
-	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, all_speeds);
-	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, all_speeds);
-	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, all_speeds);
+	if (linkmode_empty(phydev->adv_old))
+		return 0;
 
-	linkmode_andnot(not_speeds, adv_old, all_speeds);
-	linkmode_copy(supported, phydev->supported);
-	linkmode_and(speeds, supported, all_speeds);
-	linkmode_or(phydev->advertising, not_speeds, speeds);
+	linkmode_copy(adv_tmp, phydev->advertising);
+	linkmode_copy(phydev->advertising, phydev->adv_old);
+	linkmode_zero(phydev->adv_old);
 
-	if (linkmode_equal(phydev->advertising, adv_old))
+	if (linkmode_equal(phydev->advertising, adv_tmp))
 		return 0;
 
 	return phy_config_aneg(phydev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 4be6d3b47..3c2be52b1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -403,6 +403,8 @@ struct phy_device {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
+	/* used with phy_speed_down */
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv_old);
 
 	/* Energy efficient ethernet modes which should be prohibited */
 	u32 eee_broken_modes;
-- 
2.22.0

Re: [PATCH net-next 1/3] net: phy: add __set_linkmode_max_speed

[Andrew Lunn]

On Mon, Aug 12, 2019 at 11:19:31PM +0200, Heiner Kallweit wrote:
> We will need the functionality of __set_linkmode_max_speed also for
> linkmode bitmaps other than phydev->supported. Therefore split it.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/phy/phy-core.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
> index 9ae3abb2d..de085f255 100644
> --- a/drivers/net/phy/phy-core.c
> +++ b/drivers/net/phy/phy-core.c
> @@ -207,14 +207,15 @@ size_t phy_speeds(unsigned int *speeds, size_t size,
>  	return count;
>  }
>  
> -static int __set_phy_supported(struct phy_device *phydev, u32 max_speed)
> +static int __set_linkmode_max_speed(struct phy_device *phydev, u32 max_speed,
> +				    unsigned long *addr)
>  {

Hi Heiner

It looks like phydev is an unused parameter. Maybe it should be
removed?

	Andrew

Re: [PATCH net-next 1/3] net: phy: add __set_linkmode_max_speed

[Heiner Kallweit]

On 12.08.2019 23:27, Andrew Lunn wrote:
> On Mon, Aug 12, 2019 at 11:19:31PM +0200, Heiner Kallweit wrote:
>> We will need the functionality of __set_linkmode_max_speed also for
>> linkmode bitmaps other than phydev->supported. Therefore split it.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/phy/phy-core.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
>> index 9ae3abb2d..de085f255 100644
>> --- a/drivers/net/phy/phy-core.c
>> +++ b/drivers/net/phy/phy-core.c
>> @@ -207,14 +207,15 @@ size_t phy_speeds(unsigned int *speeds, size_t size,
>>  	return count;
>>  }
>>  
>> -static int __set_phy_supported(struct phy_device *phydev, u32 max_speed)
>> +static int __set_linkmode_max_speed(struct phy_device *phydev, u32 max_speed,
>> +				    unsigned long *addr)
>>  {
> 
> Hi Heiner
> 
> It looks like phydev is an unused parameter. Maybe it should be
> removed?
> 
Right, it can be removed. Thanks!

> 	Andrew
> 
Heiner

Re: [PATCH net-next 2/3] net: phy: add __phy_speed_down and phy_resolve_min_speed

[Andrew Lunn]

> +int __phy_speed_down(struct phy_device *phydev)
> +{
> +	int min_common_speed = phy_resolve_min_speed(phydev, true);
> +
> +	if (min_common_speed == SPEED_UNKNOWN)
> +		return -EINVAL;
> +
> +	return __set_linkmode_max_speed(phydev, min_common_speed,
> +					phydev->advertising);
> +}
> +
>  static void mmd_phy_indirect(struct mii_bus *bus, int phy_addr, int devad,
>  			     u16 regnum)
>  {
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 781f4810c..4be6d3b47 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -665,6 +665,7 @@ size_t phy_speeds(unsigned int *speeds, size_t size,
>  		  unsigned long *mask);
>  void of_set_phy_supported(struct phy_device *phydev);
>  void of_set_phy_eee_broken(struct phy_device *phydev);
> +int __phy_speed_down(struct phy_device *phydev);

It seems odd having a __ function exported. Can we find a better name
for it, and drop the __?

    Andrew

Re: [PATCH net-next 3/3] net: phy: let phy_speed_down/up support speeds >1Gbps

[Andrew Lunn]

On Mon, Aug 12, 2019 at 11:20:51PM +0200, Heiner Kallweit wrote:
> So far phy_speed_down/up can be used up to 1Gbps only. Remove this
> restriction by using new helper __phy_speed_down. New member adv_old
> in struct phy_device is used by phy_speed_up to restore the advertised
> modes before calling phy_speed_down. Don't simply advertise what is
> supported because a user may have intentionally removed modes from
> advertisement.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

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

    Andrew