Here is what Vladimir says about it:
at803x_aneg_done() keeps the aneg reporting as "not done" even when
the copper-side link was reported as up, but the in-band autoneg has
not finished.
That was the _intended_ behavior when that code was introduced, and
Heiner have said about it [1]:
| That's not nice from the PHY:
| It signals "link up", and if the system asks the PHY for link details,
| then it sheepishly says "well, link is *almost* up".
If the specification of phy_aneg_done behavior does not include
in-band autoneg (and it doesn't), then this piece of code does not
belong here.
The fact that we can no longer trigger this code from phylib is yet
another reason why it fails at its intended (and wrong) purpose and
should be removed.
Removing the SGMII link check, would just keep the call to
genphy_aneg_done(), which is also the fallback. Thus we can just remove
at803x_aneg_done() altogether.
[1] https://lore.kernel.org/netdev/[email protected]/
Suggested-by: Vladimir Oltean <[email protected]>
Signed-off-by: Michael Walle <[email protected]>
---
drivers/net/phy/at803x.c | 31 -------------------------------
1 file changed, 31 deletions(-)
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index c2aa4c92edde..d7799beb811c 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -751,36 +751,6 @@ static void at803x_link_change_notify(struct phy_device *phydev)
}
}
-static int at803x_aneg_done(struct phy_device *phydev)
-{
- int ccr;
-
- int aneg_done = genphy_aneg_done(phydev);
- if (aneg_done != BMSR_ANEGCOMPLETE)
- return aneg_done;
-
- /*
- * in SGMII mode, if copper side autoneg is successful,
- * also check SGMII side autoneg result
- */
- ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
- if ((ccr & AT803X_MODE_CFG_MASK) != AT803X_MODE_CFG_SGMII)
- return aneg_done;
-
- /* switch to SGMII/fiber page */
- phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr & ~AT803X_BT_BX_REG_SEL);
-
- /* check if the SGMII link is OK. */
- if (!(phy_read(phydev, AT803X_PSSR) & AT803X_PSSR_MR_AN_COMPLETE)) {
- phydev_warn(phydev, "803x_aneg_done: SGMII link is not ok\n");
- aneg_done = 0;
- }
- /* switch back to copper page */
- phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr | AT803X_BT_BX_REG_SEL);
-
- return aneg_done;
-}
-
static int at803x_read_status(struct phy_device *phydev)
{
int ss, err, old_link = phydev->link;
@@ -1198,7 +1168,6 @@ static struct phy_driver at803x_driver[] = {
.resume = at803x_resume,
/* PHY_GBIT_FEATURES */
.read_status = at803x_read_status,
- .aneg_done = at803x_aneg_done,
.config_intr = &at803x_config_intr,
.handle_interrupt = at803x_handle_interrupt,
.get_tunable = at803x_get_tunable,
--
2.20.1
Am 2021-03-18 20:44, schrieb Michael Walle:
> Here is what Vladimir says about it:
>
> at803x_aneg_done() keeps the aneg reporting as "not done" even when
> the copper-side link was reported as up, but the in-band autoneg has
> not finished.
>
> That was the _intended_ behavior when that code was introduced, and
> Heiner have said about it [1]:
>
> | That's not nice from the PHY:
> | It signals "link up", and if the system asks the PHY for link
> details,
> | then it sheepishly says "well, link is *almost* up".
>
> If the specification of phy_aneg_done behavior does not include
> in-band autoneg (and it doesn't), then this piece of code does not
> belong here.
>
> The fact that we can no longer trigger this code from phylib is yet
> another reason why it fails at its intended (and wrong) purpose and
> should be removed.
>
> Removing the SGMII link check, would just keep the call to
> genphy_aneg_done(), which is also the fallback. Thus we can just remove
> at803x_aneg_done() altogether.
>
> [1]
> https://lore.kernel.org/netdev/[email protected]/
>
> Suggested-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Michael Walle <[email protected]>
> ---
Sorry forgot the version history:
Changes since v1:
- more detailed commit message
-michael
On 18.03.2021 20:44, Michael Walle wrote:
> Here is what Vladimir says about it:
>
> at803x_aneg_done() keeps the aneg reporting as "not done" even when
> the copper-side link was reported as up, but the in-band autoneg has
> not finished.
>
> That was the _intended_ behavior when that code was introduced, and
> Heiner have said about it [1]:
>
> | That's not nice from the PHY:
> | It signals "link up", and if the system asks the PHY for link details,
> | then it sheepishly says "well, link is *almost* up".
>
> If the specification of phy_aneg_done behavior does not include
> in-band autoneg (and it doesn't), then this piece of code does not
> belong here.
>
> The fact that we can no longer trigger this code from phylib is yet
> another reason why it fails at its intended (and wrong) purpose and
> should be removed.
>
> Removing the SGMII link check, would just keep the call to
> genphy_aneg_done(), which is also the fallback. Thus we can just remove
> at803x_aneg_done() altogether.
>
> [1] https://lore.kernel.org/netdev/[email protected]/
>
> Suggested-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Michael Walle <[email protected]>
> ---
Reviewed-by: Heiner Kallweit <[email protected]>
Hello:
This patch was applied to netdev/net-next.git (refs/heads/master):
On Thu, 18 Mar 2021 20:44:31 +0100 you wrote:
> Here is what Vladimir says about it:
>
> at803x_aneg_done() keeps the aneg reporting as "not done" even when
> the copper-side link was reported as up, but the in-band autoneg has
> not finished.
>
> That was the _intended_ behavior when that code was introduced, and
> Heiner have said about it [1]:
>
> [...]
Here is the summary with links:
- [v2,net-next] net: phy: at803x: remove at803x_aneg_done()
https://git.kernel.org/netdev/net-next/c/5b6b827413e8
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html