| Message ID | 20221204174103.1033005-1-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: swphy: Support all normal speeds when link down | expand |
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;
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.
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
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?
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;
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(-)