phylib EEE updates
Subsystems (3)
Russell King - ARM Linux
reviewerv2 cover phylib EEE updates
Florian,
This series of patches depends on the previous set of changes, and is therefore net-next material.
While testing the EEE code, I discovered a number of issues:
1. It is possible to enable advertisment of EEE modes which are not supported by the hardware. We omit to check the supported modes and mask off those modes that are not supported before writing the EEE advertisment register.
2. We need to restart autonegotiation after a change of the EEE advertisment, otherwise the link partner does not see the updated EEE modes.
3. SGMII connected PHYs are also capable of supporting EEE.
I'm not too sure why we have the test in phylib for the interface mode, as the list of interface modes that support EEE is likely to increase (eg, 10G interface modes have provision to support EEE mode.) I did think about killing the test, but the older interface modes probably don't support it. Any thoughts on what to do about that - maybe the test should be reversed to exclude those modes which definitely don't support EEE?
drivers/net/phy/phy.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-)
v2: fix "phy_restart_aneg" build error identified by 0-day
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
RFC PATCH v2 1/3 net: phy: avoid setting unsupported EEE advertisments
We currently allow userspace to set any EEE advertisments it desires, whether or not the PHY supports them. For example:
# ethtool --set-eee eth1 advertise 0xffffffff
# ethtool --show-eee eth1
EEE Settings for eth1:
EEE status: disabled
Tx LPI: disabled
Supported EEE link modes: 100baseT/Full
1000baseT/Full
10000baseT/Full
Advertised EEE link modes: 100baseT/Full
1000baseT/Full
1000baseKX/Full
10000baseT/Full
10000baseKX4/Full
10000baseKR/FullClearly, this is not sane, we should only allow link modes that are supported to be advertised (as we do elsewhere.) Ensure that we mask the MDIO_AN_EEE_ADV value with the capabilities retrieved from the MDIO_PCS_EEE_ABLE register.
--- drivers/net/phy/phy.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
drivers/net/phy/phy.c +10 -5
@@ -1332,17 +1332,22 @@ EXPORT_SYMBOL(phy_ethtool_get_eee);*/ int phy_ethtool_set_eee (struct phy_device *phydev,struct ethtool_eee *data) { -int val =ethtool_adv_to_mmd_eee_adv_t (data->advertised); +int cap, adv;if (!phydev->drv)return -EIO ; -/* Mask prohibited EEE modes */ - val &= ~phydev->eee_broken_modes; +/* Get Supported EEE */ + cap =phy_read_mmd (phydev,MDIO_MMD_PCS ,MDIO_PCS_EEE_ABLE ); +if (cap <0 ) +return cap; -phy_write_mmd (phydev,MDIO_MMD_AN ,MDIO_AN_EEE_ADV , val); + adv =ethtool_adv_to_mmd_eee_adv_t (data->advertised) & cap; -return 0 ; +/* Mask prohibited EEE modes */ + adv &= ~phydev->eee_broken_modes; + +return phy_write_mmd (phydev,MDIO_MMD_AN ,MDIO_AN_EEE_ADV , adv); }EXPORT_SYMBOL (phy_ethtool_set_eee);
Signature · 1 line
2.7.4
Re: [PATCH RFC v2 1/3] net: phy: avoid setting unsupported EEE advertisments
Quoted 18 lines · Russell King
We currently allow userspace to set any EEE advertisments it desires, whether or not the PHY supports them. For example:
# ethtool --set-eee eth1 advertise 0xffffffff # ethtool --show-eee eth1 EEE Settings for eth1: EEE status: disabled Tx LPI: disabled Supported EEE link modes: 100baseT/Full 1000baseT/Full 10000baseT/Full Advertised EEE link modes: 100baseT/Full 1000baseT/Full 1000baseKX/Full 10000baseT/Full 10000baseKX4/Full 10000baseKR/Full
Quoted 6 lines
Clearly, this is not sane, we should only allow link modes that are supported to be advertised (as we do elsewhere.) Ensure that we mask the
MDIO_AN_EEE_ADVvalue with the capabilities retrieved from theMDIO_PCS_EEE_ABLEregister.Signed-off-by Russell King <[email protected]>
Signature · 1 line
Florian
RFC PATCH v2 2/3 net: phy: restart phy autonegotiation after EEE advertisment change
In reply to Russell King - ARM LinuxWhen the EEE advertisment is changed, we should restart autonegotiation to update the link partner with the new EEE settings. Add this trigger but only if the advertisment has changed.
--- drivers/net/phy/phy.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)
drivers/net/phy/phy.c +19 -2
@@ -1332,7 +1332,7 @@ EXPORT_SYMBOL(phy_ethtool_get_eee);*/ int phy_ethtool_set_eee (struct phy_device *phydev,struct ethtool_eee *data) { -int cap, adv; +int cap, old_adv, adv, ret;if (!phydev->drv)return -EIO ;
@@ -1342,12 +1342,29 @@ int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data)if (cap <0 )return cap; + old_adv =phy_read_mmd (phydev,MDIO_MMD_AN ,MDIO_AN_EEE_ADV ); +if (old_adv <0 ) +return old_adv; + adv =ethtool_adv_to_mmd_eee_adv_t (data->advertised) & cap;/* Mask prohibited EEE modes */ adv &= ~phydev->eee_broken_modes; -return phy_write_mmd (phydev,MDIO_MMD_AN ,MDIO_AN_EEE_ADV , adv); +if (old_adv != adv) { + ret =phy_write_mmd (phydev,MDIO_MMD_AN ,MDIO_AN_EEE_ADV , adv); +if (ret <0 ) +return ret; + +/* Restart autonegotiation so the new modes get sent to the +* link partner. +*/ + ret =genphy_restart_aneg (phydev); +if (ret <0 ) +return ret; + } + +return 0 ; }EXPORT_SYMBOL (phy_ethtool_set_eee);
Signature · 1 line
2.7.4
Re: [PATCH RFC v2 2/3] net: phy: restart phy autonegotiation after EEE advertisment change
Quoted 5 lines · Russell King
When the EEE advertisment is changed, we should restart autonegotiation to update the link partner with the new EEE settings. Add this trigger but only if the advertisment has changed.
Signed-off-by Russell King <[email protected]>
Signature · 1 line
Florian
RFC PATCH v2 3/3 net: phy: allow EEE with SGMII interface modes
In reply to Russell King - ARM LinuxAs EEE is able to work in SGMII mode as well, add it to the list of permissable EEE modes that phy_init_eee() will accept. This is necessary so that EEE can work with an 88E1512 connected in SGMII mode.
--- drivers/net/phy/phy.c | 1 + 1 file changed, 1 insertion(+)
drivers/net/phy/phy.c +1 -0
@@ -1215,6 +1215,7 @@ int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable)if ((phydev->duplex ==DUPLEX_FULL ) && ((phydev->interface ==PHY_INTERFACE_MODE_MII ) || (phydev->interface ==PHY_INTERFACE_MODE_GMII ) || + phydev->interface ==PHY_INTERFACE_MODE_SGMII ||phy_interface_is_rgmii (phydev) ||phy_is_internal (phydev))) {int eee_lp, eee_cap, eee_adv;
Signature · 1 line
2.7.4
Re: [PATCH RFC v2 3/3] net: phy: allow EEE with SGMII interface modes
Quoted 3 lines · Russell King
As EEE is able to work in SGMII mode as well, add it to the list of permissable EEE modes that
phy_init_eee()will accept. This is necessary so that EEE can work with an 88E1512 connected in SGMII mode.
As you mention in your cover letter, we should probably reverse this test and make it reject modes where EEE has no chance of being supported at all.
Other than that:
Signature · 1 line
Florian
Russell King - ARM Linux
reviewerRe: [PATCH RFC v2 3/3] net: phy: allow EEE with SGMII interface modes
Quoted 8 lines · Florian Fainelli
Quoted 3 lines · Russell King
As EEE is able to work in SGMII mode as well, add it to the list of permissable EEE modes that
phy_init_eee()will accept. This is necessary so that EEE can work with an 88E1512 connected in SGMII mode.As you mention in your cover letter, we should probably reverse this test and make it reject modes where EEE has no chance of being supported at all.
Want me to re-spin? Any thought on which interface modes we should explicitly exclude?
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH RFC v2 3/3] net: phy: allow EEE with SGMII interface modes
Quoted 6 lines · Russell King - ARM Linux
Quoted 5 lines · Florian Fainelli
Quoted 3 lines · Russell King
As EEE is able to work in SGMII mode as well, add it to the list of permissable EEE modes that
phy_init_eee()will accept. This is necessary so that EEE can work with an 88E1512 connected in SGMII mode.
Quoted 6 lines
Quoted 3 lines
As you mention in your cover letter, we should probably reverse this test and make it reject modes where EEE has no chance of being supported at all.
Want me to re-spin? Any thought on which interface modes we should explicitly exclude?
It actually sounds like we should just kill the check entirely, it does not appear that any of the interface mode would not fundamentally be able to support EEE, because the "lowest" mode we support is MII, and even there it's quite possible to support EEE.
Signature · 1 line
Florian
Russell King - ARM Linux
reviewerRe: [PATCH RFC v2 3/3] net: phy: allow EEE with SGMII interface modes
Quoted 11 lines · Florian Fainelli
Quoted 10 lines · Russell King - ARM Linux
Quoted 8 lines · Florian Fainelli
Quoted 3 lines · Russell King
As EEE is able to work in SGMII mode as well, add it to the list of permissable EEE modes that
phy_init_eee()will accept. This is necessary so that EEE can work with an 88E1512 connected in SGMII mode.As you mention in your cover letter, we should probably reverse this test and make it reject modes where EEE has no chance of being supported at all.
Quoted 7 lines
Quoted 2 lines
Want me to re-spin? Any thought on which interface modes we should explicitly exclude?
It actually sounds like we should just kill the check entirely, it does not appear that any of the interface mode would not fundamentally be able to support EEE, because the "lowest" mode we support is MII, and even there it's quite possible to support EEE.
Right, so it looks like the test reduces down to just:
if (phydev->duplex == DUPLEX_FULL) {agreed?
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH RFC v2 3/3] net: phy: allow EEE with SGMII interface modes
Quoted 20 lines · Russell King - ARM Linux
Quoted 11 lines · Florian Fainelli
Quoted 10 lines · Russell King - ARM Linux
Quoted 8 lines · Florian Fainelli
Quoted 3 lines · Russell King
As EEE is able to work in SGMII mode as well, add it to the list of permissable EEE modes that
phy_init_eee()will accept. This is necessary so that EEE can work with an 88E1512 connected in SGMII mode.As you mention in your cover letter, we should probably reverse this test and make it reject modes where EEE has no chance of being supported at all.
Quoted 7 lines
Quoted 2 lines
Want me to re-spin? Any thought on which interface modes we should explicitly exclude?
It actually sounds like we should just kill the check entirely, it does not appear that any of the interface mode would not fundamentally be able to support EEE, because the "lowest" mode we support is MII, and even there it's quite possible to support EEE.
Quoted 5 lines
Right, so it looks like the test reduces down to just:
if (phydev->duplex == DUPLEX_FULL) {agreed?
Yes indeed. Thanks!
Signature · 1 line
Florian
Russell King - ARM Linux
reviewerRe: [PATCH RFC v2 3/3] net: phy: allow EEE with SGMII interface modes
Here's the revised patch as requested.
Thanks.
8<===
From: Russell King <[email protected]>
Subject: [PATCH] net: phy: allow EEE with any interface mode
EEE is able to work in any PHY interface mode, there is nothing which fundamentally restricts it to only a few modes. For example, EEE works in SGMII mode with the Marvell 88E1512.
Rather than just adding SGMII mode to the list, Florian suggests removing the list of interface modes entirely:
It actually sounds like we should just kill the check entirely, it does not appear that any of the interface mode would not fundamentally be able to support EEE, because the "lowest" mode we support is MII, and even there it's quite possible to support EEE.
--- drivers/net/phy/phy.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
drivers/net/phy/phy.c +1 -8
@@ -1208,15 +1208,8 @@ int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable)return -EIO ;/* According to 802.3az,the EEE is supported only in full duplex-mode. -* Also EEE feature is active when core is operating with MII, GMII -* or RGMII (all kinds). Internal PHYs are also allowed to proceed and -* should return an error if they do not support EEE. */ -if ((phydev->duplex ==DUPLEX_FULL ) && - ((phydev->interface ==PHY_INTERFACE_MODE_MII ) || - (phydev->interface ==PHY_INTERFACE_MODE_GMII ) || -phy_interface_is_rgmii (phydev) || -phy_is_internal (phydev))) { +if (phydev->duplex ==DUPLEX_FULL ) {int eee_lp, eee_cap, eee_adv;u32 lp, cap, adv;int status;
Signature · 7 lines
2.7.4 -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Florian Fainelli
Reviewed-byRe: [PATCH RFC v2 3/3] net: phy: allow EEE with SGMII interface modes
Quoted 15 lines · Russell King - ARM Linux
Here's the revised patch as requested.
Thanks.
8<===
From: Russell King <[email protected]>
Subject: [PATCH] net: phy: allow EEE with any interface modeEEE is able to work in any PHY interface mode, there is nothing which fundamentally restricts it to only a few modes. For example, EEE works in SGMII mode with the Marvell 88E1512.
Rather than just adding SGMII mode to the list, Florian suggests removing the list of interface modes entirely:
Quoted 7 lines
It actually sounds like we should just kill the check entirely, it does not appear that any of the interface mode would not fundamentally be able to support EEE, because the "lowest" mode we support is MII, and even there it's quite possible to support EEE.
Signed-off-by Russell King <[email protected]>
I think David will require you to resubmit this as an entire patch series and without an RFC tag. Do you want to hold off a bit to get build coverage or go ahead and target net-next right away?
Thanks!
Signature · 1 line
Florian
Russell King - ARM Linux
reviewerRe: [PATCH RFC v2 3/3] net: phy: allow EEE with SGMII interface modes
Quoted 3 lines · Florian Fainelli
I think David will require you to resubmit this as an entire patch series and without an RFC tag. Do you want to hold off a bit to get build coverage or go ahead and target net-next right away?
I'd prefer to get build coverage first - there's no point in not using the facilities that are available.
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.