The negotiation of flow control / pause frame modes was broken since
commit fcf1f59afc67 ("net: phy: marvell: rearrange to use
genphy_read_lpa()") moved the setting of phydev->duplex below the
phy_resolve_aneg_pause call. Due to a check of DUPLEX_FULL in that
function, phydev->pause was no longer set.
Fix it by moving the parsing of the status variable before the blocks
dealing with the pause frames.
As the Marvell 88E1510 datasheet does not specify the timing between the
link status and the "Speed and Duplex Resolved" bit, we have to force
the link down as long as the resolved bit is not set, to avoid reporting
link up before we even have valid Speed/Duplex.
Tested with a Marvell 88E1510 (RGMII to Copper/1000Base-T)
Fixes: fcf1f59afc67 ("net: phy: marvell: rearrange to use genphy_read_lpa()")
Signed-off-by: Clemens Gruber <[email protected]>
---
Changes since v1:
- Force link to 0 if resolved bit is not set as suggested by Russell King
drivers/net/phy/marvell.c | 46 ++++++++++++++++++++-------------------
1 file changed, 24 insertions(+), 22 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 9a8badafea8a..561df5e33f65 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -1278,6 +1278,30 @@ static int marvell_read_status_page_an(struct phy_device *phydev,
int lpa;
int err;
+ if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) {
+ phydev->link = 0;
+ return 0;
+ }
+
+ if (status & MII_M1011_PHY_STATUS_FULLDUPLEX)
+ phydev->duplex = DUPLEX_FULL;
+ else
+ phydev->duplex = DUPLEX_HALF;
+
+ switch (status & MII_M1011_PHY_STATUS_SPD_MASK) {
+ case MII_M1011_PHY_STATUS_1000:
+ phydev->speed = SPEED_1000;
+ break;
+
+ case MII_M1011_PHY_STATUS_100:
+ phydev->speed = SPEED_100;
+ break;
+
+ default:
+ phydev->speed = SPEED_10;
+ break;
+ }
+
if (!fiber) {
err = genphy_read_lpa(phydev);
if (err < 0)
@@ -1306,28 +1330,6 @@ static int marvell_read_status_page_an(struct phy_device *phydev,
}
}
- if (!(status & MII_M1011_PHY_STATUS_RESOLVED))
- return 0;
-
- if (status & MII_M1011_PHY_STATUS_FULLDUPLEX)
- phydev->duplex = DUPLEX_FULL;
- else
- phydev->duplex = DUPLEX_HALF;
-
- switch (status & MII_M1011_PHY_STATUS_SPD_MASK) {
- case MII_M1011_PHY_STATUS_1000:
- phydev->speed = SPEED_1000;
- break;
-
- case MII_M1011_PHY_STATUS_100:
- phydev->speed = SPEED_100;
- break;
-
- default:
- phydev->speed = SPEED_10;
- break;
- }
-
return 0;
}
--
2.26.0
On Sat, 11 Apr 2020 18:51:25 +0200 Clemens Gruber wrote:
> The negotiation of flow control / pause frame modes was broken since
> commit fcf1f59afc67 ("net: phy: marvell: rearrange to use
> genphy_read_lpa()") moved the setting of phydev->duplex below the
> phy_resolve_aneg_pause call. Due to a check of DUPLEX_FULL in that
> function, phydev->pause was no longer set.
>
> Fix it by moving the parsing of the status variable before the blocks
> dealing with the pause frames.
>
> As the Marvell 88E1510 datasheet does not specify the timing between the
> link status and the "Speed and Duplex Resolved" bit, we have to force
> the link down as long as the resolved bit is not set, to avoid reporting
> link up before we even have valid Speed/Duplex.
>
> Tested with a Marvell 88E1510 (RGMII to Copper/1000Base-T)
>
> Fixes: fcf1f59afc67 ("net: phy: marvell: rearrange to use genphy_read_lpa()")
> Signed-off-by: Clemens Gruber <[email protected]>
> ---
> Changes since v1:
> - Force link to 0 if resolved bit is not set as suggested by Russell King
>
> drivers/net/phy/marvell.c | 46 ++++++++++++++++++++-------------------
> 1 file changed, 24 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 9a8badafea8a..561df5e33f65 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -1278,6 +1278,30 @@ static int marvell_read_status_page_an(struct phy_device *phydev,
> int lpa;
> int err;
>
> + if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) {
> + phydev->link = 0;
> + return 0;
> + }
This doesn't address my comment, so was I wrong? What I was trying to
say is that the function updates the established link info as well as
autoneg advertising info. If the link is not resolved we can't read the
link info, but we should still report the advertising modes. No?
On Sat, Apr 11, 2020 at 04:28:24PM -0700, Jakub Kicinski wrote:
> On Sat, 11 Apr 2020 18:51:25 +0200 Clemens Gruber wrote:
> > The negotiation of flow control / pause frame modes was broken since
> > commit fcf1f59afc67 ("net: phy: marvell: rearrange to use
> > genphy_read_lpa()") moved the setting of phydev->duplex below the
> > phy_resolve_aneg_pause call. Due to a check of DUPLEX_FULL in that
> > function, phydev->pause was no longer set.
> >
> > Fix it by moving the parsing of the status variable before the blocks
> > dealing with the pause frames.
> >
> > As the Marvell 88E1510 datasheet does not specify the timing between the
> > link status and the "Speed and Duplex Resolved" bit, we have to force
> > the link down as long as the resolved bit is not set, to avoid reporting
> > link up before we even have valid Speed/Duplex.
> >
> > Tested with a Marvell 88E1510 (RGMII to Copper/1000Base-T)
> >
> > Fixes: fcf1f59afc67 ("net: phy: marvell: rearrange to use genphy_read_lpa()")
> > Signed-off-by: Clemens Gruber <[email protected]>
> > ---
> > Changes since v1:
> > - Force link to 0 if resolved bit is not set as suggested by Russell King
> >
> > drivers/net/phy/marvell.c | 46 ++++++++++++++++++++-------------------
> > 1 file changed, 24 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> > index 9a8badafea8a..561df5e33f65 100644
> > --- a/drivers/net/phy/marvell.c
> > +++ b/drivers/net/phy/marvell.c
> > @@ -1278,6 +1278,30 @@ static int marvell_read_status_page_an(struct phy_device *phydev,
> > int lpa;
> > int err;
> >
> > + if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) {
> > + phydev->link = 0;
> > + return 0;
> > + }
>
> This doesn't address my comment, so was I wrong? What I was trying to
> say is that the function updates the established link info as well as
> autoneg advertising info. If the link is not resolved we can't read the
> link info, but we should still report the advertising modes. No?
If we report that the link is down, then the advertising modes are
irrelevent.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up
On Sun, 12 Apr 2020 00:45:59 +0100 Russell King - ARM Linux admin wrote:
> On Sat, Apr 11, 2020 at 04:28:24PM -0700, Jakub Kicinski wrote:
> > On Sat, 11 Apr 2020 18:51:25 +0200 Clemens Gruber wrote:
> > > The negotiation of flow control / pause frame modes was broken since
> > > commit fcf1f59afc67 ("net: phy: marvell: rearrange to use
> > > genphy_read_lpa()") moved the setting of phydev->duplex below the
> > > phy_resolve_aneg_pause call. Due to a check of DUPLEX_FULL in that
> > > function, phydev->pause was no longer set.
> > >
> > > Fix it by moving the parsing of the status variable before the blocks
> > > dealing with the pause frames.
> > >
> > > As the Marvell 88E1510 datasheet does not specify the timing between the
> > > link status and the "Speed and Duplex Resolved" bit, we have to force
> > > the link down as long as the resolved bit is not set, to avoid reporting
> > > link up before we even have valid Speed/Duplex.
> > >
> > > Tested with a Marvell 88E1510 (RGMII to Copper/1000Base-T)
> > >
> > > Fixes: fcf1f59afc67 ("net: phy: marvell: rearrange to use genphy_read_lpa()")
> > > Signed-off-by: Clemens Gruber <[email protected]>
> > > ---
> > > Changes since v1:
> > > - Force link to 0 if resolved bit is not set as suggested by Russell King
> > >
> > > drivers/net/phy/marvell.c | 46 ++++++++++++++++++++-------------------
> > > 1 file changed, 24 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> > > index 9a8badafea8a..561df5e33f65 100644
> > > --- a/drivers/net/phy/marvell.c
> > > +++ b/drivers/net/phy/marvell.c
> > > @@ -1278,6 +1278,30 @@ static int marvell_read_status_page_an(struct phy_device *phydev,
> > > int lpa;
> > > int err;
> > >
> > > + if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) {
> > > + phydev->link = 0;
> > > + return 0;
> > > + }
> >
> > This doesn't address my comment, so was I wrong? What I was trying to
> > say is that the function updates the established link info as well as
> > autoneg advertising info. If the link is not resolved we can't read the
> > link info, but we should still report the advertising modes. No?
>
> If we report that the link is down, then the advertising modes are
> irrelevent.
Ugh, these are link partner modes...
Applied, thanks!