[PATCH net-next] net: phy: swphy: Support all normal speeds when link down

[PATCH net-next] net: phy: swphy: Support all normal speeds when link down

[Andrew Lunn]

The software PHY emulator validation function is happy to accept any
link speed if the link is down. swphy_read_reg() however triggers a
WARN_ON(). Change this to report all the standard 1G link speeds are
supported. Once the speed is known the supported link modes will
change, which is a bit odd, but for emulation is probably O.K.

Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---

Hi Sean

Does this fix your problem?

drivers/net/phy/swphy.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/swphy.c b/drivers/net/phy/swphy.c
index 59f1ba4d49bc..63bd98217092 100644
--- a/drivers/net/phy/swphy.c
+++ b/drivers/net/phy/swphy.c
@@ -29,8 +29,10 @@ enum {
 	SWMII_SPEED_10 = 0,
 	SWMII_SPEED_100,
 	SWMII_SPEED_1000,
+	SWMII_SPEED_UNKNOWN,
 	SWMII_DUPLEX_HALF = 0,
 	SWMII_DUPLEX_FULL,
+	SWMII_DUPLEX_UNKNOWN,
 };
 
 /*
@@ -51,6 +53,11 @@ static const struct swmii_regs speed[] = {
 		.lpagb = LPA_1000FULL | LPA_1000HALF,
 		.estat = ESTATUS_1000_TFULL | ESTATUS_1000_THALF,
 	},
+	[SWMII_SPEED_UNKNOWN] = {
+		.bmsr  = BMSR_ESTATEN | BMSR_100FULL | BMSR_100HALF |
+			 BMSR_10FULL | BMSR_10HALF,
+		.estat = ESTATUS_1000_TFULL | ESTATUS_1000_THALF,
+	},
 };
 
 static const struct swmii_regs duplex[] = {
@@ -66,6 +73,11 @@ static const struct swmii_regs duplex[] = {
 		.lpagb = LPA_1000FULL,
 		.estat = ESTATUS_1000_TFULL,
 	},
+	[SWMII_DUPLEX_UNKNOWN] = {
+		.bmsr  = BMSR_ESTATEN | BMSR_100FULL | BMSR_100HALF |
+			 BMSR_10FULL | BMSR_10HALF,
+		.estat = ESTATUS_1000_TFULL | ESTATUS_1000_THALF,
+	},
 };
 
 static int swphy_decode_speed(int speed)
@@ -87,8 +99,9 @@ static int swphy_decode_speed(int speed)
  * @state: software phy status
  *
  * This checks that we can represent the state stored in @state can be
- * represented in the emulated MII registers.  Returns 0 if it can,
- * otherwise returns -EINVAL.
+ * represented in the emulated MII registers. Invalid speed is allowed
+ * when the link is down, but the speed must be valid when the link is
+ * up. Returns 0 if it can, otherwise returns -EINVAL.
  */
 int swphy_validate_state(const struct fixed_phy_status *state)
 {
@@ -123,11 +136,14 @@ int swphy_read_reg(int reg, const struct fixed_phy_status *state)
 	if (reg > MII_REGS_NUM)
 		return -1;
 
-	speed_index = swphy_decode_speed(state->speed);
-	if (WARN_ON(speed_index < 0))
-		return 0;
-
-	duplex_index = state->duplex ? SWMII_DUPLEX_FULL : SWMII_DUPLEX_HALF;
+	if (state->link) {
+		speed_index = swphy_decode_speed(state->speed);
+		duplex_index = state->duplex ? SWMII_DUPLEX_FULL :
+			SWMII_DUPLEX_HALF;
+	} else {
+		speed_index = SWMII_SPEED_UNKNOWN;
+		duplex_index = SWMII_DUPLEX_UNKNOWN;
+	}
 
 	bmsr |= speed[speed_index].bmsr & duplex[duplex_index].bmsr;
 	estat |= speed[speed_index].estat & duplex[duplex_index].estat;
-- 
2.38.1

Re: [PATCH net-next] net: phy: swphy: Support all normal speeds when link down

[Sean Anderson]

On 12/4/22 12:41, Andrew Lunn wrote:
> The software PHY emulator validation function is happy to accept any
> link speed if the link is down. swphy_read_reg() however triggers a
> WARN_ON(). Change this to report all the standard 1G link speeds are
> supported. Once the speed is known the supported link modes will
> change, which is a bit odd, but for emulation is probably O.K.
> 
> Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
> 
> Hi Sean
> 
> Does this fix your problem?

Well, it removes the WARN, which is all I was after.

--Sean

> drivers/net/phy/swphy.c | 30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/phy/swphy.c b/drivers/net/phy/swphy.c
> index 59f1ba4d49bc..63bd98217092 100644
> --- a/drivers/net/phy/swphy.c
> +++ b/drivers/net/phy/swphy.c
> @@ -29,8 +29,10 @@ enum {
>  	SWMII_SPEED_10 = 0,
>  	SWMII_SPEED_100,
>  	SWMII_SPEED_1000,
> +	SWMII_SPEED_UNKNOWN,
>  	SWMII_DUPLEX_HALF = 0,
>  	SWMII_DUPLEX_FULL,
> +	SWMII_DUPLEX_UNKNOWN,
>  };
>  
>  /*
> @@ -51,6 +53,11 @@ static const struct swmii_regs speed[] = {
>  		.lpagb = LPA_1000FULL | LPA_1000HALF,
>  		.estat = ESTATUS_1000_TFULL | ESTATUS_1000_THALF,
>  	},
> +	[SWMII_SPEED_UNKNOWN] = {
> +		.bmsr  = BMSR_ESTATEN | BMSR_100FULL | BMSR_100HALF |
> +			 BMSR_10FULL | BMSR_10HALF,
> +		.estat = ESTATUS_1000_TFULL | ESTATUS_1000_THALF,
> +	},
>  };
>  
>  static const struct swmii_regs duplex[] = {
> @@ -66,6 +73,11 @@ static const struct swmii_regs duplex[] = {
>  		.lpagb = LPA_1000FULL,
>  		.estat = ESTATUS_1000_TFULL,
>  	},
> +	[SWMII_DUPLEX_UNKNOWN] = {
> +		.bmsr  = BMSR_ESTATEN | BMSR_100FULL | BMSR_100HALF |
> +			 BMSR_10FULL | BMSR_10HALF,
> +		.estat = ESTATUS_1000_TFULL | ESTATUS_1000_THALF,
> +	},
>  };
>  
>  static int swphy_decode_speed(int speed)
> @@ -87,8 +99,9 @@ static int swphy_decode_speed(int speed)
>   * @state: software phy status
>   *
>   * This checks that we can represent the state stored in @state can be
> - * represented in the emulated MII registers.  Returns 0 if it can,
> - * otherwise returns -EINVAL.
> + * represented in the emulated MII registers. Invalid speed is allowed
> + * when the link is down, but the speed must be valid when the link is
> + * up. Returns 0 if it can, otherwise returns -EINVAL.
>   */
>  int swphy_validate_state(const struct fixed_phy_status *state)
>  {
> @@ -123,11 +136,14 @@ int swphy_read_reg(int reg, const struct fixed_phy_status *state)
>  	if (reg > MII_REGS_NUM)
>  		return -1;
>  
> -	speed_index = swphy_decode_speed(state->speed);
> -	if (WARN_ON(speed_index < 0))
> -		return 0;
> -
> -	duplex_index = state->duplex ? SWMII_DUPLEX_FULL : SWMII_DUPLEX_HALF;
> +	if (state->link) {
> +		speed_index = swphy_decode_speed(state->speed);
> +		duplex_index = state->duplex ? SWMII_DUPLEX_FULL :
> +			SWMII_DUPLEX_HALF;
> +	} else {
> +		speed_index = SWMII_SPEED_UNKNOWN;
> +		duplex_index = SWMII_DUPLEX_UNKNOWN;
> +	}
>  
>  	bmsr |= speed[speed_index].bmsr & duplex[duplex_index].bmsr;
>  	estat |= speed[speed_index].estat & duplex[duplex_index].estat;

Re: [PATCH net-next] net: phy: swphy: Support all normal speeds when link down

[Russell King (Oracle)]

On Sun, Dec 04, 2022 at 06:41:03PM +0100, Andrew Lunn wrote:
> The software PHY emulator validation function is happy to accept any
> link speed if the link is down. swphy_read_reg() however triggers a
> WARN_ON(). Change this to report all the standard 1G link speeds are
> supported. Once the speed is known the supported link modes will
> change, which is a bit odd, but for emulation is probably O.K.
> 
> Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>

This isn't what I suggested. I suggested restoring the old behaviour of
fixed_phy before commit 5ae68b0ce134 ("phy: move fixed_phy MII register
generation to a library") which did _not_ report all speeds, but
reported no supported speeds in BMSR.

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

Re: [PATCH net-next] net: phy: swphy: Support all normal speeds when link down

[Andrew Lunn]

On Mon, Dec 05, 2022 at 04:04:21PM +0000, Russell King (Oracle) wrote:
> On Sun, Dec 04, 2022 at 06:41:03PM +0100, Andrew Lunn wrote:
> > The software PHY emulator validation function is happy to accept any
> > link speed if the link is down. swphy_read_reg() however triggers a
> > WARN_ON(). Change this to report all the standard 1G link speeds are
> > supported. Once the speed is known the supported link modes will
> > change, which is a bit odd, but for emulation is probably O.K.
> > 
> > Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
> 
> This isn't what I suggested. I suggested restoring the old behaviour of
> fixed_phy before commit 5ae68b0ce134 ("phy: move fixed_phy MII register
> generation to a library") which did _not_ report all speeds, but
> reported no supported speeds in BMSR.

O.K.

Which is better. No speeds, or all speeds? I think all speeds is more
like what a real PHY does.

     Andrew

Re: [PATCH net-next] net: phy: swphy: Support all normal speeds when link down

[Russell King (Oracle)]

On Mon, Dec 05, 2022 at 05:44:19PM +0100, Andrew Lunn wrote:
> On Mon, Dec 05, 2022 at 04:04:21PM +0000, Russell King (Oracle) wrote:
> > On Sun, Dec 04, 2022 at 06:41:03PM +0100, Andrew Lunn wrote:
> > > The software PHY emulator validation function is happy to accept any
> > > link speed if the link is down. swphy_read_reg() however triggers a
> > > WARN_ON(). Change this to report all the standard 1G link speeds are
> > > supported. Once the speed is known the supported link modes will
> > > change, which is a bit odd, but for emulation is probably O.K.
> > > 
> > > Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
> > 
> > This isn't what I suggested. I suggested restoring the old behaviour of
> > fixed_phy before commit 5ae68b0ce134 ("phy: move fixed_phy MII register
> > generation to a library") which did _not_ report all speeds, but
> > reported no supported speeds in BMSR.
> 
> O.K.
> 
> Which is better. No speeds, or all speeds? I think all speeds is more
> like what a real PHY does.

We have a precedent for reporting no speeds - that's the behaviour of
fixed_phy before the above mentioned commit. So, if it was good enough
for many years of fixed_phy, shouldn't it still be good enough?

I guess it ultimately depends how those ethernet drivers making use
of fixed_phy with phylib end up behaving - will phylib operate
correctly, or does it read the BMSR and ESTATUS to determine the
speeds now, but didn't before?

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